Re: [Intel-gfx] [PATCH v2] drm: avoid races with modesetting rights

[Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>]

On 16/8/21 9:59 pm, Daniel Vetter wrote:
> On Mon, Aug 16, 2021 at 12:31 PM Desmond Cheong Zhi Xi
> <desmondcheongzx@gmail.com> wrote:
>>
>> On 16/8/21 5:04 pm, Daniel Vetter wrote:
>>> On Mon, Aug 16, 2021 at 10:53 AM Desmond Cheong Zhi Xi
>>> <desmondcheongzx@gmail.com> wrote:
>>>> On 16/8/21 2:47 am, kernel test robot wrote:
>>>>> Hi Desmond,
>>>>>
>>>>> Thank you for the patch! Yet something to improve:
>>>>>
>>>>> [auto build test ERROR on next-20210813]
>>>>> [also build test ERROR on v5.14-rc5]
>>>>> [cannot apply to linus/master v5.14-rc5 v5.14-rc4 v5.14-rc3]
>>>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>>>> And when submitting patch, we suggest to use '--base' as documented in
>>>>> https://git-scm.com/docs/git-format-patch]
>>>>>
>>>>> url:    https://github.com/0day-ci/linux/commits/Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
>>>>> base:    4b358aabb93a2c654cd1dcab1a25a589f6e2b153
>>>>> config: i386-randconfig-a004-20210815 (attached as .config)
>>>>> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>>>>> reproduce (this is a W=1 build):
>>>>>            # https://github.com/0day-ci/linux/commit/cf6d8354b7d7953cd866fad004cbb189adfa074f
>>>>>            git remote add linux-review https://github.com/0day-ci/linux
>>>>>            git fetch --no-tags linux-review Desmond-Cheong-Zhi-Xi/drm-avoid-races-with-modesetting-rights/20210815-234145
>>>>>            git checkout cf6d8354b7d7953cd866fad004cbb189adfa074f
>>>>>            # save the attached .config to linux build tree
>>>>>            make W=1 ARCH=i386
>>>>>
>>>>> If you fix the issue, kindly add following tag as appropriate
>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>>
>>>>> All errors (new ones prefixed by >>, old ones prefixed by <<):
>>>>>
>>>>>>> ERROR: modpost: "task_work_add" [drivers/gpu/drm/drm.ko] undefined!
>>>>>
>>>>
>>>> I'm a bit uncertain about this. Looking into the .config used, this
>>>> error seems to happen because task_work_add isn't an exported symbol,
>>>> but DRM is being compiled as a loadable kernel module (CONFIG_DRM=m).
>>>>
>>>> One way to deal with this is to export the symbol, but there was a
>>>> proposed patch to do this a few months back that wasn't picked up [1],
>>>> so I'm not sure what to make of this.
>>>>
>>>> I'll export the symbol as part of a v3 series, and check in with the
>>>> task-work maintainers.
>>>>
>>>> Link:
>>>> https://lore.kernel.org/lkml/20210127150029.13766-3-joshi.k@samsung.com/ [1]
>>>
>>> Yeah that sounds best. I have two more thoughts on the patch:
>>> - drm_master_flush isn't used by any modules outside of drm.ko, so we
>>> can unexport it and drop the kerneldoc (the comment is still good).
>>> These kind of internal functions have their declaration in
>>> drm-internal.h - there's already a few there from drm_auth.c
>>>
>>
>> Sounds good, I'll do that and move the declaration from drm_auth.h to
>> drm_internal.h.
>>
>>> - We know have 3 locks for master state, that feels a bit like
>>> overkill. The spinlock I think we need to keep due to lock inversions,
>>> but the master_mutex and master_rwsem look like we should be able to
>>> merge them? I.e. anywhere we currently grab the master_mutex we could
>>> instead grab the rwsem in either write mode (when we change stuff) or
>>> read mode (when we just check, like in master_internal_acquire).
>>>
>>> Thoughts?
>>> -Daniel
>>>
>>
>> Using rwsem in the places where we currently hold the mutex seems pretty
>> doable.
>>
>> There are some tricky bits once we add rwsem read locks to the ioctl
>> handler. Some ioctl functions like drm_authmagic need a write lock.
> 
> Ah yes, I only looked at the dropmaster/setmaster ioctl, and those
> don't have the DRM_MASTER bit set.
> 
>> In this particular case, it might make sense to break master_mutex down
>> into finer-grained locks, since the function doesn't change master
>> permissions. It just needs to prevent concurrent writes to the
>> drm_master.magic_map idr.
> 
> Yeah for authmagic we could perhaps just reuse the spinlock to protect
> ->magic_map?
> 

Yup, I had to move the spinlock from struct drm_file to struct 
drm_device, but I think that should work.

>> For other ioctls, I'll take a closer look on a case-by-case basis.
> 
> If it's too much shuffling then I think totally fine to leave things
> as-is. Just feels a bit silly to have 3 locks, on of which is an
> rwlock itself, for this fairly small amount of state.
> -Daniel
> 

Agreed, there's a lot of overlap between the master_mutex and rwsem so 
this a good opportunity to refactor things.

I'm cleaning up a v3 series now. There's some movement, but most of it 
are fixes to potential bugs that I saw while refactoring. We can see if 
the new version is a better design.

>>
>>>>
>>>>> ---
>>>>> 0-DAY CI Kernel Test Service, Intel Corporation
>>>>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>>>>>
>>>>
>>>
>>>
>>
> 
>