[PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Asahi Lina]

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(-)

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
-- 
2.35.1

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Javier Martinez Canillas]

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?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Asahi Lina]

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

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Javier Martinez Canillas]

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! 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4358 bytes --]

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Asahi Lina]

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

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Javier Martinez Canillas]

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!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

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

-- 
Best regards,
Dmitry

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 610 bytes --]

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

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[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?

~~ Lina

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1299 bytes --]

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[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?

~~ Lina

Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2018 bytes --]

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]