Re: [igt-dev] [PATCH i-g-t] lib/ioctl_wrappers: Assert that prime_handle_to_fd returns a valid fd

[Petri Latvala <petri.latvala@intel.com>]

On Tue, Mar 12, 2019 at 01:14:17PM +0000, Chris Wilson wrote:
> Quoting Petri Latvala (2019-03-12 13:00:04)
> > On Tue, Mar 12, 2019 at 11:34:37AM +0000, Chris Wilson wrote:
> > > Quoting Petri Latvala (2019-03-12 11:21:14)
> > > > If the ioctl is successful, the returned fd should be valid. Check
> > > > that it is, thus also helping static analysis in almost 70 call sites.
> > > > 
> > > > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > > > ---
> > > >  lib/ioctl_wrappers.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > > > index 39920f87..31969e77 100644
> > > > --- a/lib/ioctl_wrappers.c
> > > > +++ b/lib/ioctl_wrappers.c
> > > > @@ -1332,6 +1332,7 @@ int prime_handle_to_fd(int fd, uint32_t handle)
> > > >         args.fd = -1;
> > > >  
> > > >         do_ioctl(fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &args);
> > > > +       igt_assert_fd(args.fd);
> > > 
> > > Why? Why would we be using a static analysis tool that complains about
> > > feeding invalid data to the kernel, because that is exactly the goal of
> > > igt.
> > 
> > Because after teaching it what data should be invalid and what should
> > be valid, it has already revealed a couple of bugs?
> 
> Did the kernel handle the invalid fd correctly? That is what is
> important. It's not a bug for igt to call close(-1) or whatever.

But it is a bug in IGT if you call a function foo() and expect it to
do error handling for you, and it giving you stuff you'd still need to
check yourself.

Imagine PRIME_HANDLE_TO_FD ioctl being broken and giving out fd -5. It
would show up in test results, but pointing to further ioctls failing
with EINVAL.

> > Are you objecting to checking that the ioctl gives an fd that is >= 0?
> > Where's the line here, what parts of the uapi semantics must always be
> > left unchecked in a test suite for uapi?
> 
> No, my opinion is htat this assertion is only valid in a test not library
> code, as the library code should just be a conduit and even the do_ioctl
> is a mistake as there is no higher level wrapper here.

Is that tested in a test currently, I didn't search thoroughly? All
call sites I checked just use the returned fd as it was valid.

Library code should just be a conduit, sure. But it's also a valid
expectation that functions that are supposed to automatically
fail/skip for you on errors do them for all occasions that can cause
the library code to fail their postcondition of valid data. It's the
whole reason we have functions typically in pairs of foo() and
__foo().

How do we go forward?

1) Splash in an __prime_handle_to_fd() that will return the fd
directly, make prime_handle_to_fd call that and assert that the ioctl
succeeded and the fd is >= 0

2) Use igt_assume() instead for the fd, point to a test that checks
the validness of the fd on a successful ioctl in the commit message


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev