| Message ID | 20230205125124.2260-1-lina@asahilina.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt() | expand |
Hello Lina, On 2/5/23 13:51, Asahi Lina wrote: > Other functions touching shmem->sgt take the pages lock, so do that here > too. drm_gem_shmem_get_pages() & co take the same lock, so move to the > _locked() variants to avoid recursive locking. > > Discovered while auditing locking to write the Rust abstractions. > > Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") > Fixes: 4fa3d66f132b ("drm/shmem: Do dma_unmap_sg before purging pages") > Signed-off-by: Asahi Lina <lina@asahilina.net> > --- Good catch. The patch looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> What about drm_gem_shmem_free() BTW, I believe that the helper should also grab the lock before unmap / free the sgtable?
On 07/02/2023 03.47, Javier Martinez Canillas wrote: > Hello Lina, > > On 2/5/23 13:51, Asahi Lina wrote: >> Other functions touching shmem->sgt take the pages lock, so do that here >> too. drm_gem_shmem_get_pages() & co take the same lock, so move to the >> _locked() variants to avoid recursive locking. >> >> Discovered while auditing locking to write the Rust abstractions. >> >> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") >> Fixes: 4fa3d66f132b ("drm/shmem: Do dma_unmap_sg before purging pages") >> Signed-off-by: Asahi Lina <lina@asahilina.net> >> --- > > Good catch. The patch looks good to me. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > What about drm_gem_shmem_free() BTW, I believe that the helper should also > grab the lock before unmap / free the sgtable? That's called from driver free callbacks, so it should only be called when there are no other users left and the refcount is zero, right? If there's anyone else racing it I think we have bigger problems than the pages lock at that point, since the last thing it does is `kfree(shmem);` ^^ (In Rust terms this is equivalent to the Drop trait, which takes a mutable/exclusive reference, which means no other reference to the object can exist at that point, so no races are possible. And in fact in my Rust abstraction I trigger a drop of the Rust object embedded in the shmem object before calling drm_gem_shmem_free(), so if this invariant doesn't hold that code would be wrong too!) ~~ Lina
On 2/7/23 11:33, Asahi Lina wrote: > On 07/02/2023 03.47, Javier Martinez Canillas wrote: >> Hello Lina, >> >> On 2/5/23 13:51, Asahi Lina wrote: >>> Other functions touching shmem->sgt take the pages lock, so do that here >>> too. drm_gem_shmem_get_pages() & co take the same lock, so move to the >>> _locked() variants to avoid recursive locking. >>> >>> Discovered while auditing locking to write the Rust abstractions. >>> >>> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") >>> Fixes: 4fa3d66f132b ("drm/shmem: Do dma_unmap_sg before purging pages") >>> Signed-off-by: Asahi Lina <lina@asahilina.net> >>> --- >> >> Good catch. The patch looks good to me. >> >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >> >> What about drm_gem_shmem_free() BTW, I believe that the helper should also >> grab the lock before unmap / free the sgtable? > > That's called from driver free callbacks, so it should only be called > when there are no other users left and the refcount is zero, right? If > there's anyone else racing it I think we have bigger problems than the > pages lock at that point, since the last thing it does is `kfree(shmem);` ^^ > Yes, I was wondering only for the critical section that does: if (shmem->sgt) { dma_unmap_sgtable(obj->dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0); sg_free_table(shmem->sgt); kfree(shmem->sgt); } if (shmem->pages) drm_gem_shmem_put_pages(shmem); > (In Rust terms this is equivalent to the Drop trait, which takes a > mutable/exclusive reference, which means no other reference to the > object can exist at that point, so no races are possible. And in fact in > my Rust abstraction I trigger a drop of the Rust object embedded in the > shmem object before calling drm_gem_shmem_free(), so if this invariant > doesn't hold that code would be wrong too!) > But I guess you are correct and would be safe to assume that the .free callback won't race against other struct drm_gem_object_funcs handlers. I just felt to ask since wasn't sure about that. I'll wait a few days in case someone else wants to take a look to your patch and then push it to drm-misc. Thanks again!
Hi Am 05.02.23 um 13:51 schrieb Asahi Lina: > Other functions touching shmem->sgt take the pages lock, so do that here Really? I was just locking at the Lima driver and it apparently access sgt without locking in [1]. Not that I disagree with the patch in general. Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/lima/lima_gem.c#L21 > too. drm_gem_shmem_get_pages() & co take the same lock, so move to the > _locked() variants to avoid recursive locking. > > Discovered while auditing locking to write the Rust abstractions. > > Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") > Fixes: 4fa3d66f132b ("drm/shmem: Do dma_unmap_sg before purging pages") > Signed-off-by: Asahi Lina <lina@asahilina.net> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 54 ++++++++++++++++---------- > 1 file changed, 34 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index b602cd72a120..2c559b310cad 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -681,23 +681,7 @@ struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem) > } > EXPORT_SYMBOL_GPL(drm_gem_shmem_get_sg_table); > > -/** > - * drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a > - * scatter/gather table for a shmem GEM object. > - * @shmem: shmem GEM object > - * > - * This function returns a scatter/gather table suitable for driver usage. If > - * the sg table doesn't exist, the pages are pinned, dma-mapped, and a sg > - * table created. > - * > - * This is the main function for drivers to get at backing storage, and it hides > - * and difference between dma-buf imported and natively allocated objects. > - * drm_gem_shmem_get_sg_table() should not be directly called by drivers. > - * > - * Returns: > - * A pointer to the scatter/gather table of pinned pages or errno on failure. > - */ > -struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) > +static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_object *shmem) > { > struct drm_gem_object *obj = &shmem->base; > int ret; > @@ -708,7 +692,7 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) > > WARN_ON(obj->import_attach); > > - ret = drm_gem_shmem_get_pages(shmem); > + ret = drm_gem_shmem_get_pages_locked(shmem); > if (ret) > return ERR_PTR(ret); > > @@ -730,10 +714,40 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) > sg_free_table(sgt); > kfree(sgt); > err_put_pages: > - drm_gem_shmem_put_pages(shmem); > + drm_gem_shmem_put_pages_locked(shmem); > return ERR_PTR(ret); > } > -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt); > + > +/** > + * drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a > + * scatter/gather table for a shmem GEM object. > + * @shmem: shmem GEM object > + * > + * This function returns a scatter/gather table suitable for driver usage. If > + * the sg table doesn't exist, the pages are pinned, dma-mapped, and a sg > + * table created. > + * > + * This is the main function for drivers to get at backing storage, and it hides > + * and difference between dma-buf imported and natively allocated objects. > + * drm_gem_shmem_get_sg_table() should not be directly called by drivers. > + * > + * Returns: > + * A pointer to the scatter/gather table of pinned pages or errno on failure. > + */ > +struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) > +{ > + int ret; > + struct sg_table *sgt; > + > + ret = mutex_lock_interruptible(&shmem->pages_lock); > + if (ret) > + return ERR_PTR(ret); > + sgt = drm_gem_shmem_get_pages_sgt_locked(shmem); > + mutex_unlock(&shmem->pages_lock); > + > + return sgt; > +} > +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt); > > /** > * drm_gem_shmem_prime_import_sg_table - Produce a shmem GEM object from
Sorry, I accidentally sent this reply offlist! Resending, my apologies... On 07/02/2023 20.29, Thomas Zimmermann wrote: > Hi > > Am 05.02.23 um 13:51 schrieb Asahi Lina: >> Other functions touching shmem->sgt take the pages lock, so do that here > > Really? I was just locking at the Lima driver and it apparently access > sgt without locking in [1]. Not that I disagree with the patch in general. It looks like that lima code is reimplementing a lot of helper functionality. I imagine it was written before the helpers...? I think most of that function could be replaced with a call to drm_gem_shmem_get_pages_sgt(). I don't know exactly how the lima driver works, but if there is a possibility of multiple calls to lima_heap_alloc() on the same BO without a higher-level mutex protecting it, I would say that code is racy. For the Rust abstraction (and really for a well-designed API in general) you want a coherent story on locking, so I think it makes sense to take the pages lock to manipulate the sgt, since drm_gem_shmem_get_pages_sgt() was already taking the pages lock for inner things anyway. Otherwise it's impossible to make safe without adding another discrete layer of locking around everything (I can't just take the pages lock in the wrapper since drm_gem_shmem_get_pages_sgt() would try to recursively lock it). > Best regards > Thomas > > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/lima/lima_gem.c#L21 ~~ Lina
On 2/6/23 19:47, Javier Martinez Canillas wrote: > Hello Lina, > > On 2/5/23 13:51, Asahi Lina wrote: >> Other functions touching shmem->sgt take the pages lock, so do that here >> too. drm_gem_shmem_get_pages() & co take the same lock, so move to the >> _locked() variants to avoid recursive locking. >> >> Discovered while auditing locking to write the Rust abstractions. >> >> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") >> Fixes: 4fa3d66f132b ("drm/shmem: Do dma_unmap_sg before purging pages") >> Signed-off-by: Asahi Lina <lina@asahilina.net> >> --- > > Good catch. The patch looks good to me. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Pushed this to drm-misc (drm-misc-next). Thanks!
On 2/5/23 15:51, Asahi Lina wrote: > -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt); > +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt); Note it was a GPL symbol. I expect that all drm-shmem exports should be GPL.
Hi Am 25.02.23 um 22:51 schrieb Dmitry Osipenko: > On 2/5/23 15:51, Asahi Lina wrote: >> -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt); >> +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt); > > Note it was a GPL symbol. I expect that all drm-shmem exports should be GPL. Right. I didn't notice that change, but it's probably not allowed. This needs to be reverted to a GPL export Best regards Thomas >
On 27/02/2023 16.45, Thomas Zimmermann wrote: > Hi > > Am 25.02.23 um 22:51 schrieb Dmitry Osipenko: >> On 2/5/23 15:51, Asahi Lina wrote: >>> -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt); >>> +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt); >> >> Note it was a GPL symbol. I expect that all drm-shmem exports should be GPL. > > Right. I didn't notice that change, but it's probably not allowed. This > needs to be reverted to a GPL export I'm sorry, this was not intentional! I think I removed and re-added the export as part of making the wrapper and didn't notice it used to be _GPL... Do you want me to send a patch to add it back? ~~ Lina
Hi Am 27.02.23 um 08:55 schrieb Asahi Lina: > On 27/02/2023 16.45, Thomas Zimmermann wrote: >> Hi >> >> Am 25.02.23 um 22:51 schrieb Dmitry Osipenko: >>> On 2/5/23 15:51, Asahi Lina wrote: >>>> -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt); >>>> +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt); >>> >>> Note it was a GPL symbol. I expect that all drm-shmem exports should be GPL. >> >> Right. I didn't notice that change, but it's probably not allowed. This >> needs to be reverted to a GPL export > > I'm sorry, this was not intentional! I think I removed and re-added the > export as part of making the wrapper and didn't notice it used to be _GPL... > > Do you want me to send a patch to add it back? Yes, please do. The Fixes tag is Fixes: ddddedaa0db9 ("drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()") This commit is in drm-misc-next-fixes. But the branch is closed already as we're in the middle of the merge window. I think it's best to merge the fix through drm-misc-fixes after the -rc1 hs been tagged. Best regards Thomas > > ~~ Lina
On 27/02/2023 17.04, Thomas Zimmermann wrote: > Hi > > Am 27.02.23 um 08:55 schrieb Asahi Lina: >> On 27/02/2023 16.45, Thomas Zimmermann wrote: >>> Hi >>> >>> Am 25.02.23 um 22:51 schrieb Dmitry Osipenko: >>>> On 2/5/23 15:51, Asahi Lina wrote: >>>>> -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt); >>>>> +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt); >>>> >>>> Note it was a GPL symbol. I expect that all drm-shmem exports should be GPL. >>> >>> Right. I didn't notice that change, but it's probably not allowed. This >>> needs to be reverted to a GPL export >> >> I'm sorry, this was not intentional! I think I removed and re-added the >> export as part of making the wrapper and didn't notice it used to be _GPL... >> >> Do you want me to send a patch to add it back? > > Yes, please do. The Fixes tag is > > Fixes: ddddedaa0db9 ("drm/shmem-helper: Fix locking for > drm_gem_shmem_get_pages_sgt()") > > This commit is in drm-misc-next-fixes. But the branch is closed already > as we're in the middle of the merge window. I think it's best to merge > the fix through drm-misc-fixes after the -rc1 hs been tagged. Sent! I also noticed that there are quite a few other non-GPL exports in the file, with no real logic that I can see... I'm guessing most of those weren't intentional either? ~~ Lina
Hi Am 27.02.23 um 10:07 schrieb Asahi Lina: > On 27/02/2023 17.04, Thomas Zimmermann wrote: >> Hi >> >> Am 27.02.23 um 08:55 schrieb Asahi Lina: >>> On 27/02/2023 16.45, Thomas Zimmermann wrote: >>>> Hi >>>> >>>> Am 25.02.23 um 22:51 schrieb Dmitry Osipenko: >>>>> On 2/5/23 15:51, Asahi Lina wrote: >>>>>> -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt); >>>>>> +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt); >>>>> >>>>> Note it was a GPL symbol. I expect that all drm-shmem exports should be GPL. >>>> >>>> Right. I didn't notice that change, but it's probably not allowed. This >>>> needs to be reverted to a GPL export >>> >>> I'm sorry, this was not intentional! I think I removed and re-added the >>> export as part of making the wrapper and didn't notice it used to be _GPL... >>> >>> Do you want me to send a patch to add it back? >> >> Yes, please do. The Fixes tag is >> >> Fixes: ddddedaa0db9 ("drm/shmem-helper: Fix locking for >> drm_gem_shmem_get_pages_sgt()") >> >> This commit is in drm-misc-next-fixes. But the branch is closed already >> as we're in the middle of the merge window. I think it's best to merge >> the fix through drm-misc-fixes after the -rc1 hs been tagged. > > Sent! I also noticed that there are quite a few other non-GPL exports in > the file, with no real logic that I can see... I'm guessing most of > those weren't intentional either? I don't know. My guess is that some authors used EXPORT_SYMBOL() out of habit and others used EXPORT_SYMBOL_GPL() intentionally. And now, it's chaotic. Even the file's initial commit 2194a63a818d contains both. I assume that some of the code has been taken from the DMA helpers, which date back much earlier with _GPL-only exports (see commit b9d474500546). Best regards Thomas > > ~~ Lina
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index b602cd72a120..2c559b310cad 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -681,23 +681,7 @@ struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem) } EXPORT_SYMBOL_GPL(drm_gem_shmem_get_sg_table); -/** - * drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a - * scatter/gather table for a shmem GEM object. - * @shmem: shmem GEM object - * - * This function returns a scatter/gather table suitable for driver usage. If - * the sg table doesn't exist, the pages are pinned, dma-mapped, and a sg - * table created. - * - * This is the main function for drivers to get at backing storage, and it hides - * and difference between dma-buf imported and natively allocated objects. - * drm_gem_shmem_get_sg_table() should not be directly called by drivers. - * - * Returns: - * A pointer to the scatter/gather table of pinned pages or errno on failure. - */ -struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) +static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_object *shmem) { struct drm_gem_object *obj = &shmem->base; int ret; @@ -708,7 +692,7 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) WARN_ON(obj->import_attach); - ret = drm_gem_shmem_get_pages(shmem); + ret = drm_gem_shmem_get_pages_locked(shmem); if (ret) return ERR_PTR(ret); @@ -730,10 +714,40 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) sg_free_table(sgt); kfree(sgt); err_put_pages: - drm_gem_shmem_put_pages(shmem); + drm_gem_shmem_put_pages_locked(shmem); return ERR_PTR(ret); } -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt); + +/** + * drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a + * scatter/gather table for a shmem GEM object. + * @shmem: shmem GEM object + * + * This function returns a scatter/gather table suitable for driver usage. If + * the sg table doesn't exist, the pages are pinned, dma-mapped, and a sg + * table created. + * + * This is the main function for drivers to get at backing storage, and it hides + * and difference between dma-buf imported and natively allocated objects. + * drm_gem_shmem_get_sg_table() should not be directly called by drivers. + * + * Returns: + * A pointer to the scatter/gather table of pinned pages or errno on failure. + */ +struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem) +{ + int ret; + struct sg_table *sgt; + + ret = mutex_lock_interruptible(&shmem->pages_lock); + if (ret) + return ERR_PTR(ret); + sgt = drm_gem_shmem_get_pages_sgt_locked(shmem); + mutex_unlock(&shmem->pages_lock); + + return sgt; +} +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt); /** * drm_gem_shmem_prime_import_sg_table - Produce a shmem GEM object from
Other functions touching shmem->sgt take the pages lock, so do that here too. drm_gem_shmem_get_pages() & co take the same lock, so move to the _locked() variants to avoid recursive locking. Discovered while auditing locking to write the Rust abstractions. Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") Fixes: 4fa3d66f132b ("drm/shmem: Do dma_unmap_sg before purging pages") Signed-off-by: Asahi Lina <lina@asahilina.net> --- drivers/gpu/drm/drm_gem_shmem_helper.c | 54 ++++++++++++++++---------- 1 file changed, 34 insertions(+), 20 deletions(-)