[PATCH 07/12] drm: drop redundant drm_file->is_master

[David Herrmann]

Hi

On Fri, Jul 25, 2014 at 9:56 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Thu, Jul 24, 2014 at 11:38:28PM +0200, David Herrmann wrote:
>> On Thu, Jul 24, 2014 at 11:52 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> > On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote:
>> >> +static inline bool drm_is_master(const struct drm_file *file)
>> >> +{
>> >
>> > Hm, we don't have any means to synchronize is_master checks with
>> > concurrent ioctls and stuff. Do we care? Orthogonal issue really.
>>
>> We don't.. My drm-master reworks contains a comment about that. It's
>> not really problematic as all ioctls run for a determinate time in
>> kernel-code except for drm_read(), but drm_read() is per-file, not
>> per-device, so we're fine. However, with unfortunate timings, we might
>> really end up with problems.
>>
>> vmwgfx solves this by using separate locks and verifying them against
>> the current master. it's not perfect and I'm not sure I like it better
>> than no locks, but at least they were aware of the problem.
>>
>> Btw., the only thing I know how to solve it properly is to make
>> "master_mutex" an rwlock and all f_op entries take the read-lock,
>> while master modifications take the write-lock. Not sure it's worth
>> it, though.
>
> Imo that's terrible. And I'm not even sure we need to care, e.g. if we do
> a master switch to a different compositor any ioctl the new compositor
> does will grab some locks, which will force the old ioctl to complete.
>
> Well mostly, and only if we don't do lockless tricks or lock-dropping and
> it's racy.

There is always a race! Like this:

CPU A:
1: enter drm_ioctl()
2: check file->is_master
3: enter drm_some_ioctl()
4: acquire some DRM internal locks

If CPU B acquires DRM-Master between step 2 and 3, CPU A might execute
an ioctl with an arbitrary delay. Maybe CPU B even executed some ioctl
after acquiring DRM-Master before CPU A had the chance to enter the
ioctl it's about to execute.

Not that I care much, but we have to remember that those races always
exist. Given that DRM-Master is privileged, this is not really
high-priority to fix..

> I guess a proper fix would be to wait for all ioctls on a device to
> complete. The vfs doesn't have any cool infrastructure for this as part of
> the general revoke work that we could reuse?

What the VFS rework does is this:

if (!atomic_inc_unless_negative(file->sth))
        return -ENODEV;
ret = file->f_op->some_op();
atomic_dec(file->sth);
return ret;

That is, it wraps all calls to a file-operation with an
atomic-counter. However, there's only one counter per open-file, not
one per file-operation. If we'd want that for DRM-Master, we couldn't
use it as otherwise all file-operations would be blocked. Furthermore,
VFS only allows disabling an open-file. Once disabled, you cannot
enable it again.

So I don't think a read/write lock is a bad idea. RCU doesn't work as
our ioctls are way to heavy for rcu_read_lock(), SRCU is basically
rw-sem for our use-case. A hand-crafted atomic counter is also
equivalent to rw-sem. So yeah, it might lock nasty, but any other
solution will just hide the fact that we have a read/write lock.

Anyhow, I'm not working on a fix, so if no-one else looks at it, we
can just ignore it.

Thanks
David