clover: Fix host access validation for sub-buffer creation

Submitted by Aaron Watry on April 7, 2018, 6:47 p.m.

Details

Message ID 20180407184737.26185-1-awatry@gmail.com
State New
Headers show
Series "clover: Fix host access validation for sub-buffer creation" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Aaron Watry April 7, 2018, 6:47 p.m.
From CL 1.2 Section 5.2.1:
    CL_INVALID_VALUE if buffer was created with CL_MEM_HOST_WRITE_ONLY and
    flags specify CL_MEM_HOST_READ_ONLY , or if buffer was created with
    CL_MEM_HOST_READ_ONLY and flags specify CL_MEM_HOST_WRITE_ONLY , or if
    buffer was created with CL_MEM_HOST_NO_ACCESS and flags specify
    CL_MEM_HOST_READ_ONLY or CL_MEM_HOST_WRITE_ONLY .

Fixes CL 1.2 CTS test/api get_buffer_info

Signed-off-by: Aaron Watry <awatry@gmail.com>
Cc: Francisco Jerez <currojerez@riseup.net>
---
 src/gallium/state_trackers/clover/api/memory.cpp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp
index 9b3cd8b1f5..451e8a8c56 100644
--- a/src/gallium/state_trackers/clover/api/memory.cpp
+++ b/src/gallium/state_trackers/clover/api/memory.cpp
@@ -57,10 +57,14 @@  namespace {
                                       parent.flags() & host_access_flags) |
                                      (parent.flags() & host_ptr_flags));
 
-         if (~flags & parent.flags() &
-             ((dev_access_flags & ~CL_MEM_READ_WRITE) | host_access_flags))
+         if (~flags & parent.flags() & (dev_access_flags & ~CL_MEM_READ_WRITE))
             throw error(CL_INVALID_VALUE);
 
+         //Check if new host access flags cause a mismatch between host-read/write-only.
+         const cl_mem_flags new_flags = flags & ~(parent.flags()) & ~CL_MEM_HOST_NO_ACCESS;
+         if (new_flags & host_access_flags & parent.flags())
+            throw error (CL_INVALID_VALUE);
+
          return flags;
 
       } else {

Comments

Aaron Watry <awatry@gmail.com> writes:

>   From CL 1.2 Section 5.2.1:
>     CL_INVALID_VALUE if buffer was created with CL_MEM_HOST_WRITE_ONLY and
>     flags specify CL_MEM_HOST_READ_ONLY , or if buffer was created with
>     CL_MEM_HOST_READ_ONLY and flags specify CL_MEM_HOST_WRITE_ONLY , or if
>     buffer was created with CL_MEM_HOST_NO_ACCESS and flags specify
>     CL_MEM_HOST_READ_ONLY or CL_MEM_HOST_WRITE_ONLY .
>
> Fixes CL 1.2 CTS test/api get_buffer_info
>

What combination of flags is the test-case providing for both the
parent and sub buffer?

> Signed-off-by: Aaron Watry <awatry@gmail.com>
> Cc: Francisco Jerez <currojerez@riseup.net>
> ---
>  src/gallium/state_trackers/clover/api/memory.cpp | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp
> index 9b3cd8b1f5..451e8a8c56 100644
> --- a/src/gallium/state_trackers/clover/api/memory.cpp
> +++ b/src/gallium/state_trackers/clover/api/memory.cpp
> @@ -57,10 +57,14 @@ namespace {
>                                        parent.flags() & host_access_flags) |
>                                       (parent.flags() & host_ptr_flags));
>  
> -         if (~flags & parent.flags() &
> -             ((dev_access_flags & ~CL_MEM_READ_WRITE) | host_access_flags))
> +         if (~flags & parent.flags() & (dev_access_flags & ~CL_MEM_READ_WRITE))
>              throw error(CL_INVALID_VALUE);
>  
> +         //Check if new host access flags cause a mismatch between host-read/write-only.
> +         const cl_mem_flags new_flags = flags & ~(parent.flags()) & ~CL_MEM_HOST_NO_ACCESS;
> +         if (new_flags & host_access_flags & parent.flags())
> +            throw error (CL_INVALID_VALUE);
> +

This doesn't look correct to me, the condition will always evaluate to
zero, you're calculating the conjunction of ~parent.flags() and
parent.flags() which is zero, so the error will never be emitted.

>           return flags;
>  
>        } else {
> -- 
> 2.14.1
On Mon, Apr 16, 2018, 5:24 PM Francisco Jerez <currojerez@riseup.net> wrote:

> Aaron Watry <awatry@gmail.com> writes:
>
> >   From CL 1.2 Section 5.2.1:
> >     CL_INVALID_VALUE if buffer was created with CL_MEM_HOST_WRITE_ONLY
> and
> >     flags specify CL_MEM_HOST_READ_ONLY , or if buffer was created with
> >     CL_MEM_HOST_READ_ONLY and flags specify CL_MEM_HOST_WRITE_ONLY , or
> if
> >     buffer was created with CL_MEM_HOST_NO_ACCESS and flags specify
> >     CL_MEM_HOST_READ_ONLY or CL_MEM_HOST_WRITE_ONLY .
> >
> > Fixes CL 1.2 CTS test/api get_buffer_info
> >
>
> What combination of flags is the test-case providing for both the
> parent and sub buffer?
>

The original motivation for this was a CTS test that was creating a sub
buffer with flags of:
CL_MEM_HOST_NO_ACCESS | CL_MEM_READ_WRITE

With a parent buffer created as:
CL_MEM_HOST_READ_ONLY | CL_MEM_READ_WRITE

Which according to my reading of the spec should be allowed.

>
> > Signed-off-by: Aaron Watry <awatry@gmail.com>
> > Cc: Francisco Jerez <currojerez@riseup.net>
> > ---
> >  src/gallium/state_trackers/clover/api/memory.cpp | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/gallium/state_trackers/clover/api/memory.cpp
> b/src/gallium/state_trackers/clover/api/memory.cpp
> > index 9b3cd8b1f5..451e8a8c56 100644
> > --- a/src/gallium/state_trackers/clover/api/memory.cpp
> > +++ b/src/gallium/state_trackers/clover/api/memory.cpp
> > @@ -57,10 +57,14 @@ namespace {
> >                                        parent.flags() &
> host_access_flags) |
> >                                       (parent.flags() & host_ptr_flags));
> >
> > -         if (~flags & parent.flags() &
> > -             ((dev_access_flags & ~CL_MEM_READ_WRITE) |
> host_access_flags))
> > +         if (~flags & parent.flags() & (dev_access_flags &
> ~CL_MEM_READ_WRITE))
> >              throw error(CL_INVALID_VALUE);
> >
> > +         //Check if new host access flags cause a mismatch between
> host-read/write-only.
> > +         const cl_mem_flags new_flags = flags & ~(parent.flags()) &
> ~CL_MEM_HOST_NO_ACCESS;
> > +         if (new_flags & host_access_flags & parent.flags())
> > +            throw error (CL_INVALID_VALUE);
> > +
>
> This doesn't look correct to me, the condition will always evaluate to
> zero, you're calculating the conjunction of ~parent.flags() and
> parent.flags() which is zero, so the error will never be emitted.
>

I'll see what I can do. I agree with a fresh reading that it looks fishy at
best.

--Aaron

>
> >           return flags;
> >
> >        } else {
> > --
> > 2.14.1
>
Aaron Watry <awatry@gmail.com> writes:

> On Mon, Apr 16, 2018, 5:24 PM Francisco Jerez <currojerez@riseup.net> wrote:
>
>> Aaron Watry <awatry@gmail.com> writes:
>>
>> >   From CL 1.2 Section 5.2.1:
>> >     CL_INVALID_VALUE if buffer was created with CL_MEM_HOST_WRITE_ONLY
>> and
>> >     flags specify CL_MEM_HOST_READ_ONLY , or if buffer was created with
>> >     CL_MEM_HOST_READ_ONLY and flags specify CL_MEM_HOST_WRITE_ONLY , or
>> if
>> >     buffer was created with CL_MEM_HOST_NO_ACCESS and flags specify
>> >     CL_MEM_HOST_READ_ONLY or CL_MEM_HOST_WRITE_ONLY .
>> >
>> > Fixes CL 1.2 CTS test/api get_buffer_info
>> >
>>
>> What combination of flags is the test-case providing for both the
>> parent and sub buffer?
>>
>
> The original motivation for this was a CTS test that was creating a sub
> buffer with flags of:
> CL_MEM_HOST_NO_ACCESS | CL_MEM_READ_WRITE
>
> With a parent buffer created as:
> CL_MEM_HOST_READ_ONLY | CL_MEM_READ_WRITE
>
> Which according to my reading of the spec should be allowed.
>

Right, I see.

>>
>> > Signed-off-by: Aaron Watry <awatry@gmail.com>
>> > Cc: Francisco Jerez <currojerez@riseup.net>
>> > ---
>> >  src/gallium/state_trackers/clover/api/memory.cpp | 8 ++++++--
>> >  1 file changed, 6 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/src/gallium/state_trackers/clover/api/memory.cpp
>> b/src/gallium/state_trackers/clover/api/memory.cpp
>> > index 9b3cd8b1f5..451e8a8c56 100644
>> > --- a/src/gallium/state_trackers/clover/api/memory.cpp
>> > +++ b/src/gallium/state_trackers/clover/api/memory.cpp
>> > @@ -57,10 +57,14 @@ namespace {
>> >                                        parent.flags() &
>> host_access_flags) |
>> >                                       (parent.flags() & host_ptr_flags));
>> >
>> > -         if (~flags & parent.flags() &
>> > -             ((dev_access_flags & ~CL_MEM_READ_WRITE) |
>> host_access_flags))
>> > +         if (~flags & parent.flags() & (dev_access_flags &
>> ~CL_MEM_READ_WRITE))
>> >              throw error(CL_INVALID_VALUE);
>> >

I think you want to keep the hunk above and then do something along the
lines of:

+         if (!(flags & CL_MEM_HOST_NO_ACCESS) &&
+             (~flags & parent.flags() & host_access_flags))
+            throw error(CL_INVALID_VALUE);

>> > +         //Check if new host access flags cause a mismatch between
>> host-read/write-only.
>> > +         const cl_mem_flags new_flags = flags & ~(parent.flags()) &
>> ~CL_MEM_HOST_NO_ACCESS;
>> > +         if (new_flags & host_access_flags & parent.flags())
>> > +            throw error (CL_INVALID_VALUE);
>> > +
>>
>> This doesn't look correct to me, the condition will always evaluate to
>> zero, you're calculating the conjunction of ~parent.flags() and
>> parent.flags() which is zero, so the error will never be emitted.
>>
>
> I'll see what I can do. I agree with a fresh reading that it looks fishy at
> best.
>
> --Aaron
>
>>
>> >           return flags;
>> >
>> >        } else {
>> > --
>> > 2.14.1
>>