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

[Daniel Vetter <daniel@ffwll.ch>]

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

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?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch