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

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

[Petri Latvala]

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);
 
 	return args.fd;
 }
-- 
2.19.1

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

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

[Chris Wilson]

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.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Petri Latvala]

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?

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?


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

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

[Chris Wilson]

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.
 
> 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.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Petri Latvala]

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

[igt-dev] ✗ Fi.CI.BAT: failure for lib/ioctl_wrappers: Assert that prime_handle_to_fd returns a valid fd

[Patchwork]

== Series Details ==

Series: lib/ioctl_wrappers: Assert that prime_handle_to_fd returns a valid fd
URL   : https://patchwork.freedesktop.org/series/57873/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5735 -> IGTPW_2594
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_2594 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2594, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/57873/revisions/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_2594:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_hangcheck:
    - fi-cfl-8700k:       PASS -> DMESG-FAIL

  
Known issues
------------

  Here are the changes found in IGTPW_2594 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_basic@gtt-bsd2:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] +57

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       NOTRUN -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_evict:
    - fi-bsw-kefka:       PASS -> DMESG-WARN [fdo#107709]

  * igt@kms_addfb_basic@addfb25-y-tiled-small:
    - fi-byt-n2820:       NOTRUN -> SKIP [fdo#109271] +56

  * igt@kms_busy@basic-flip-a:
    - fi-bsw-n3050:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_busy@basic-flip-c:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-byt-n2820:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-bsw-n3050:       NOTRUN -> SKIP [fdo#109271] +62

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-hsw-peppy:       NOTRUN -> SKIP [fdo#109271] +46

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       NOTRUN -> DMESG-FAIL [fdo#102614] / [fdo#107814]
    - fi-byt-clapper:     NOTRUN -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     NOTRUN -> FAIL [fdo#103191] / [fdo#107362]

  * igt@kms_psr@primary_mmap_gtt:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +8

  * igt@runner@aborted:
    - fi-bsw-kefka:       NOTRUN -> FAIL [fdo#107709]

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      FAIL [fdo#108511] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@prime_vgem@basic-fence-flip:
    - fi-gdg-551:         DMESG-FAIL [fdo#103182] -> PASS
    - fi-kbl-7567u:       FAIL [fdo#104008] -> PASS

  
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107814]: https://bugs.freedesktop.org/show_bug.cgi?id=107814
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (41 -> 40)
------------------------------

  Additional (4): fi-hsw-peppy fi-byt-n2820 fi-byt-clapper fi-bsw-n3050 
  Missing    (5): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-bdw-samus 


Build changes
-------------

    * IGT: IGT_4881 -> IGTPW_2594

  CI_DRM_5735: a51930d5783a7de342db8ea03199fcc0ab31c098 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2594: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2594/
  IGT_4881: 6ef8aa586de6ea899dd567378ba0f76885bdd914 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2594/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Daniel Vetter]

On Tue, Mar 12, 2019 at 01:21:14PM +0200, Petri Latvala wrote:
> 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);

fd >= 0 if we want this check. I think it's ok to have the assert here, since
do_ioctl already checks that the ioctl didn't fail. If we want an
unchecked version we can add the usual __prime_hanlde_to_fd.
-Daniel

>  
>  	return args.fd;
>  }
> -- 
> 2.19.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Petri Latvala]

On Tue, Mar 12, 2019 at 04:25:33PM +0100, Daniel Vetter wrote:
> On Tue, Mar 12, 2019 at 01:21:14PM +0200, Petri Latvala wrote:
> > 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);
> 
> fd >= 0 if we want this check. I think it's ok to have the assert here, since
> do_ioctl already checks that the ioctl didn't fail. If we want an
> unchecked version we can add the usual __prime_hanlde_to_fd.

fd >= 0 is what igt_assert_fd does?


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

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

[Daniel Vetter]

On Wed, Mar 13, 2019 at 10:59:09AM +0200, Petri Latvala wrote:
> On Tue, Mar 12, 2019 at 04:25:33PM +0100, Daniel Vetter wrote:
> > On Tue, Mar 12, 2019 at 01:21:14PM +0200, Petri Latvala wrote:
> > > 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);
> > 
> > fd >= 0 if we want this check. I think it's ok to have the assert here, since
> > do_ioctl already checks that the ioctl didn't fail. If we want an
> > unchecked version we can add the usual __prime_hanlde_to_fd.
> 
> fd >= 0 is what igt_assert_fd does?

I'm blind :-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev