[kbuild-all] Re: [PATCH v2] drm: avoid races with modesetting rights

On Mon, Aug 16, 2021 at 12:31 PM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com&gt; 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(a)gmail.com&gt; 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-... >>>> 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/cf6d8354b7d7953cd866fad004cbb189a... >>>> 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(a)intel.com&gt; >>>> >>>> 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?

> 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

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