linux-kernel - Re: [PATCH 5/5] drm: Add four ioctls for managing drm mode object leases [v6]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <878tgawzil.fsf@keithp.com>
Date:   Mon, 16 Oct 2017 14:31:30 -0700
From:   "Keith Packard" <keithp@...thp.com>
To:     Sean Paul <seanpaul@...omium.org>
Cc:     linux-kernel@...r.kernel.org, Dave Airlie <airlied@...hat.com>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 5/5] drm: Add four ioctls for managing drm mode object leases [v6]

Sean Paul <seanpaul@...omium.org> writes:

> nit: space before ,

Thanks.

>> +	/* Clone the lessor file to create a new file for us */
>> +	DRM_DEBUG_LEASE("Allocating lease file\n");
>> +	path_get(&lessor_file->f_path);
>
> Please forgive the stupid question, but where is this reference given
> up?

That's not a stupid question, it's a very subtle one which took me quite
a while to sort out. Here's path_get:

        void path_get(const struct path *path)
        {
        	mntget(path->mnt);
        	dget(path->dentry);
        }

So, getting a reference on a 'path' actually gets a reference on two of
the things it points to.

alloc_file is passed the path and doesn't take an additional reference
on either of these fields, presumably because the normal path has the
caller taking a reference while looking up the object and handing that
reference off to alloc_file. In our case, we're creating a new file that
refers to the same path as an existing one, so we need another
reference.

When the file is finally freed in __fput, the two references are dropped
at the end of the function:

        static void __fput(struct file *file)
        {
        	struct dentry *dentry = file->f_path.dentry;
        	struct vfsmount *mnt = file->f_path.mnt;

                ...

               	dput(dentry);
        	mntput(mnt);
        }

This was probably the twistiest part of creating a lease. All of the DRM
stuff was trivial; getting the core kernel object reference counts right
was a pain.

>> +	if (lessee->lessor == NULL)
>> +		/* owner can use all objects */
>> +		object_idr = &lessee->dev->mode_config.crtc_idr;
>
> What about other types of objects?

If I understand your question correctly, the answer is that 'crtc_idr'
is misnamed -- it holds all of the mode setting objects.

Thanks for your review, let me know if you have more questions!

-- 
-keith

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)