Re: [igt-dev] [PATCH i-g-t 3/9] tests/kms_lease: invalid corner-cases for create-lease ioctl

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Feb 20, 2019 at 03:43:45PM -0500, Lyude Paul wrote:
> On Wed, 2019-02-20 at 17:25 +0100, Daniel Vetter wrote:
> > Found a few things in the kernel that looks suspicious, separate
> > patches on their way.
> > 
> > I also reviewed coverage for list-lesses and get-lease, and coverage
> > seems complete for these.
> > 
> > Cc: Keith Packard <keithp@keithp.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  tests/kms_lease.c | 100
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 100 insertions(+)
> > 
> > diff --git a/tests/kms_lease.c b/tests/kms_lease.c
> > index fa97f6e11334..4062a0b365c9 100644
> > --- a/tests/kms_lease.c
> > +++ b/tests/kms_lease.c
> > @@ -623,6 +623,103 @@ static void run_test(data_t *data, void
> > (*testfunc)(data_t *))
> >  		      "no valid crtc/connector combinations found\n");
> >  }
> >  
> > +static void invalid_create_leases(data_t *data)
> > +{
> > +	uint32_t object_ids[4];
> > +	struct local_drm_mode_create_lease mcl;
> > +	drmModeRes *resources;
> > +	int tmp_fd;
> > +
> > +	/* empty lease */
> > +	mcl.object_ids = 0;
> > +	mcl.object_count = 0;
> > +	mcl.flags = 0;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EINVAL);
> > +
> > +	/* NULL array pointer */
> > +	mcl.object_count = 1;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EFAULT);
> > +
> > +	/* nil object */
> > +	object_ids[0] = 0;
> > +	mcl.object_ids = (uint64_t) (uintptr_t) object_ids;
> > +	mcl.object_count = 1;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -ENOENT);
> > +
> > +	/* no crtc, non-universal_plane */
> > +	drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
> > +	object_ids[0] = data->master.display.outputs[0].id;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EINVAL);
> > +
> > +	/* no connector, non-universal_plane */
> > +	object_ids[0] = data->master.display.pipes[0].crtc_id;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EINVAL);
> > +
> > +	/* sanity check */
> > +	object_ids[0] = data->master.display.pipes[0].crtc_id;
> > +	object_ids[1] = data->master.display.outputs[0].id;
> > +	mcl.object_count = 2;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), 0);
> > +	close(mcl.fd);
> > +
> > +	/* no plane, universal planes */
> > +	drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EINVAL);
> > +
> > +	/* sanity check */
> > +	object_ids[2] = igt_pipe_get_plane_type(&data-
> > >master.display.pipes[0],
> > +						DRM_PLANE_TYPE_PRIMARY)-
> > >drm_plane->plane_id;
> > +	mcl.object_count = 3;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), 0);
> > +	close(mcl.fd);
> > +
> > +	/* array overflow */
> > +	mcl.object_count = UINT32_MAX / sizeof(object_ids[0]) + 1;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -ENOMEM);
> > +	mcl.object_count = UINT32_MAX / sizeof(object_ids[0]) + 2;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -ENOMEM);
> > +	mcl.object_count = UINT32_MAX / sizeof(object_ids[0]) + 3;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -ENOMEM);
> > +	mcl.object_count = UINT32_MAX / sizeof(object_ids[0]) + 4;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -ENOMEM);
> 
> nitpick: could probably make this a loop, up to you

Yeah, will do.

> 
> > +
> > +	/* invalid flags */
> > +	mcl.object_count = 3;
> > +	mcl.flags = O_CLOEXEC | O_NONBLOCK;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), 0);
> > +	close(mcl.fd);
> 
> I think I'm missing something. The hunk below this makes sense, but I'm not
> quite sure I understand what's happening above here? ^

I like to sprinkle in sanity check tests, to make sure I'm actually
testing what I'm testing. First run something you know should work, then
change one thing (and one thing only), then you'll know the kernel fails
because of that one thing you're testing, and not some strange interaction
which might hide bugs.

I'll add a /* sanity check */ here and move the /* invalid flags */ down.
-Daniel

> 
> Besides that, the rest of this patch looks good to me
> 
> > +	mcl.flags = -1;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EINVAL);
> > +
> > +	/* no subleasing */
> > +	mcl.object_count = 3;
> > +	mcl.flags = 0;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), 0);
> > +	tmp_fd = mcl.fd;
> > +	igt_assert_eq(create_lease(tmp_fd, &mcl), -EINVAL);
> > +	close(tmp_fd);
> > +
> > +	/* no double-leasing */
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), 0);
> > +	tmp_fd = mcl.fd;
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EBUSY);
> > +	close(tmp_fd);
> > +
> > +	/* no double leasing */
> > +	object_ids[3] = object_ids[2];
> > +	mcl.object_count = 4;
> > +	/* Note: the ENOSPC is from idr double-insertion failing */
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -ENOSPC);
> > +
> > +	/* no encoder leasing */
> > +	resources = drmModeGetResources(data->master.fd);
> > +	igt_assert(resources);
> > +	igt_assert(resources->count_encoders > 0);
> > +	object_ids[3] = resources->encoders[0];
> > +	igt_assert_eq(create_lease(data->master.fd, &mcl), -EINVAL);
> > +	drmModeFreeResources(resources);
> > +}
> > +
> >  igt_main
> >  {
> >  	data_t data;
> > @@ -655,4 +752,7 @@ igt_main
> >  			run_test(&data, f->func);
> >  		}
> >  	}
> > +
> > +	igt_subtest("invalid-create-leases")
> > +		invalid_create_leases(&data);
> >  }
> -- 
> Cheers,
> 	Lyude Paul
> 

-- 
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