[PATCH -next] um: vector: use GFP_ATOMIC under spin lock

[PATCH -next] um: vector: use GFP_ATOMIC under spin lock

[Wei Yongjun]

A spin lock is taken here so we should use GFP_ATOMIC.

Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 arch/um/drivers/vector_kern.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
index 92617e16829e..6ff0065a271d 100644
--- a/arch/um/drivers/vector_kern.c
+++ b/arch/um/drivers/vector_kern.c
@@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
 		kfree(vp->bpf->filter);
 		vp->bpf->filter = NULL;
 	} else {
-		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL);
+		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC);
 		if (vp->bpf == NULL) {
 			netdev_err(dev, "failed to allocate memory for firmware\n");
 			goto flash_fail;
@@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
 	if (request_firmware(&fw, efl->data, &vdevice->pdev.dev))
 		goto flash_fail;
 
-	vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_KERNEL);
+	vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_ATOMIC);
 	if (!vp->bpf->filter)
 		goto free_buffer;

Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock

[Anton Ivanov]

On 28/11/2019 02:01, Wei Yongjun wrote:
> A spin lock is taken here so we should use GFP_ATOMIC.
> 
> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
>   arch/um/drivers/vector_kern.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
> index 92617e16829e..6ff0065a271d 100644
> --- a/arch/um/drivers/vector_kern.c
> +++ b/arch/um/drivers/vector_kern.c
> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
>   		kfree(vp->bpf->filter);
>   		vp->bpf->filter = NULL;
>   	} else {
> -		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL);
> +		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC);
>   		if (vp->bpf == NULL) {
>   			netdev_err(dev, "failed to allocate memory for firmware\n");
>   			goto flash_fail;
> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
>   	if (request_firmware(&fw, efl->data, &vdevice->pdev.dev))
>   		goto flash_fail;
>   
> -	vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_KERNEL);
> +	vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_ATOMIC);
>   	if (!vp->bpf->filter)
>   		goto free_buffer;
> 
> 
> 
> 

Acked-by: Anton Ivanov <anton.ivanov@cambridgegreys.co.uk>

> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
> 


-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock

[Dan Carpenter]

On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote:
> A spin lock is taken here so we should use GFP_ATOMIC.
> 
> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
>  arch/um/drivers/vector_kern.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
> index 92617e16829e..6ff0065a271d 100644
> --- a/arch/um/drivers/vector_kern.c
> +++ b/arch/um/drivers/vector_kern.c
> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
>  		kfree(vp->bpf->filter);
>  		vp->bpf->filter = NULL;
>  	} else {
> -		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL);
> +		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC);
>  		if (vp->bpf == NULL) {
>  			netdev_err(dev, "failed to allocate memory for firmware\n");
>  			goto flash_fail;
> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
>  	if (request_firmware(&fw, efl->data, &vdevice->pdev.dev))
            ^^^^^^^^^^^^^^^^

Is it really possible to call request_firmware() while holding a
spin_lock?  I was so sure that read from the disk.

regards,
dan carpenter

>  		goto flash_fail;
>  
> -	vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_KERNEL);
> +	vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_ATOMIC);
>  	if (!vp->bpf->filter)
>  		goto free_buffer;
> 
> 

Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock

[Anton Ivanov]

On 28/11/2019 08:06, Dan Carpenter wrote:
> On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote:
>> A spin lock is taken here so we should use GFP_ATOMIC.
>>
>> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>> ---
>>   arch/um/drivers/vector_kern.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
>> index 92617e16829e..6ff0065a271d 100644
>> --- a/arch/um/drivers/vector_kern.c
>> +++ b/arch/um/drivers/vector_kern.c
>> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
>>   		kfree(vp->bpf->filter);
>>   		vp->bpf->filter = NULL;
>>   	} else {
>> -		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL);
>> +		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC);
>>   		if (vp->bpf == NULL) {
>>   			netdev_err(dev, "failed to allocate memory for firmware\n");
>>   			goto flash_fail;
>> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
>>   	if (request_firmware(&fw, efl->data, &vdevice->pdev.dev))
>              ^^^^^^^^^^^^^^^^
> 
> Is it really possible to call request_firmware() while holding a
> spin_lock?  I was so sure that read from the disk.

Works, I tested the patch quite a few times.


> 
> regards,
> dan carpenter
> 
>>   		goto flash_fail;
>>   
>> -	vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_KERNEL);
>> +	vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_ATOMIC);
>>   	if (!vp->bpf->filter)
>>   		goto free_buffer;
>>
>>
> 
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
> 

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock

[Dan Carpenter]

On Thu, Nov 28, 2019 at 08:18:30AM +0000, Anton Ivanov wrote:
> 
> 
> On 28/11/2019 08:06, Dan Carpenter wrote:
> > On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote:
> > > A spin lock is taken here so we should use GFP_ATOMIC.
> > > 
> > > Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
> > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> > > ---
> > >   arch/um/drivers/vector_kern.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
> > > index 92617e16829e..6ff0065a271d 100644
> > > --- a/arch/um/drivers/vector_kern.c
> > > +++ b/arch/um/drivers/vector_kern.c
> > > @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
> > >   		kfree(vp->bpf->filter);
> > >   		vp->bpf->filter = NULL;
> > >   	} else {
> > > -		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL);
> > > +		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC);
> > >   		if (vp->bpf == NULL) {
> > >   			netdev_err(dev, "failed to allocate memory for firmware\n");
> > >   			goto flash_fail;
> > > @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
> > >   	if (request_firmware(&fw, efl->data, &vdevice->pdev.dev))
> >              ^^^^^^^^^^^^^^^^
> > 
> > Is it really possible to call request_firmware() while holding a
> > spin_lock?  I was so sure that read from the disk.
> 
> Works, I tested the patch quite a few times.
> 

Do you have CONFIG_DEBUG_ATOMIC_SLEEP enabled?  The GFP_KERNEL calls
should have triggered a warning if so.

regards,
dan carpenter

Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "anton ivanov" <anton.ivanov@cambridgegreys.com>
> An: "Dan Carpenter" <dan.carpenter@oracle.com>, "Wei Yongjun" <weiyongjun1@huawei.com>
> CC: "Song Liu" <songliubraving@fb.com>, "Daniel Borkmann" <daniel@iogearbox.net>, "kernel-janitors"
> <kernel-janitors@vger.kernel.org>, "richard" <richard@nod.at>, "Jeff Dike" <jdike@addtoit.com>, "linux-um"
> <linux-um@lists.infradead.org>, "Alexei Starovoitov" <ast@kernel.org>, "netdev" <netdev@vger.kernel.org>,
> bpf@vger.kernel.org, "Martin KaFai Lau" <kafai@fb.com>
> Gesendet: Donnerstag, 28. November 2019 09:18:30
> Betreff: Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock

> On 28/11/2019 08:06, Dan Carpenter wrote:
>> On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote:
>>> A spin lock is taken here so we should use GFP_ATOMIC.
>>>
>>> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>> ---
>>>   arch/um/drivers/vector_kern.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
>>> index 92617e16829e..6ff0065a271d 100644
>>> --- a/arch/um/drivers/vector_kern.c
>>> +++ b/arch/um/drivers/vector_kern.c
>>> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device
>>> *dev,
>>>   		kfree(vp->bpf->filter);
>>>   		vp->bpf->filter = NULL;
>>>   	} else {
>>> -		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL);
>>> +		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC);
>>>   		if (vp->bpf == NULL) {
>>>   			netdev_err(dev, "failed to allocate memory for firmware\n");
>>>   			goto flash_fail;
>>> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device
>>> *dev,
>>>   	if (request_firmware(&fw, efl->data, &vdevice->pdev.dev))
>>              ^^^^^^^^^^^^^^^^
>> 
>> Is it really possible to call request_firmware() while holding a
>> spin_lock?  I was so sure that read from the disk.
> 
> Works, I tested the patch quite a few times.

It works because of the nature of UML ->no  SMP or PREEMPT.
But better request the firmware before taking the spinlock.
request_firmware() can block.
Same for the kmalloc(), just allocate the buffer before and then assign
the pointer under the lock. That way you don't need GFP_ATOMIC.

Thanks,
//richard

Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock

[Anton Ivanov]

On 28/11/2019 08:37, Dan Carpenter wrote:
> On Thu, Nov 28, 2019 at 08:18:30AM +0000, Anton Ivanov wrote:
>>
>>
>> On 28/11/2019 08:06, Dan Carpenter wrote:
>>> On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote:
>>>> A spin lock is taken here so we should use GFP_ATOMIC.
>>>>
>>>> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
>>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>>> ---
>>>>    arch/um/drivers/vector_kern.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
>>>> index 92617e16829e..6ff0065a271d 100644
>>>> --- a/arch/um/drivers/vector_kern.c
>>>> +++ b/arch/um/drivers/vector_kern.c
>>>> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
>>>>    		kfree(vp->bpf->filter);
>>>>    		vp->bpf->filter = NULL;
>>>>    	} else {
>>>> -		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL);
>>>> +		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC);
>>>>    		if (vp->bpf == NULL) {
>>>>    			netdev_err(dev, "failed to allocate memory for firmware\n");
>>>>    			goto flash_fail;
>>>> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev,
>>>>    	if (request_firmware(&fw, efl->data, &vdevice->pdev.dev))
>>>               ^^^^^^^^^^^^^^^^
>>>
>>> Is it really possible to call request_firmware() while holding a
>>> spin_lock?  I was so sure that read from the disk.
>>
>> Works, I tested the patch quite a few times.
>>
> 
> Do you have CONFIG_DEBUG_ATOMIC_SLEEP enabled?  The GFP_KERNEL calls
> should have triggered a warning if so.

I do not think we can use that in um.

config DEBUG_ATOMIC_SLEEP
         bool "Sleep inside atomic section checking"
         select PREEMPT_COUNT
         depends on DEBUG_KERNEL
         depends on !ARCH_NO_PREEMPT

In arch/um/Kconfig

         select ARCH_NO_PREEMPT

Brgds,

> 
> regards,
> dan carpenter
> 
> 
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
> 

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock

[Anton Ivanov]

On 28/11/2019 08:41, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "anton ivanov" <anton.ivanov@cambridgegreys.com>
>> An: "Dan Carpenter" <dan.carpenter@oracle.com>, "Wei Yongjun" <weiyongjun1@huawei.com>
>> CC: "Song Liu" <songliubraving@fb.com>, "Daniel Borkmann" <daniel@iogearbox.net>, "kernel-janitors"
>> <kernel-janitors@vger.kernel.org>, "richard" <richard@nod.at>, "Jeff Dike" <jdike@addtoit.com>, "linux-um"
>> <linux-um@lists.infradead.org>, "Alexei Starovoitov" <ast@kernel.org>, "netdev" <netdev@vger.kernel.org>,
>> bpf@vger.kernel.org, "Martin KaFai Lau" <kafai@fb.com>
>> Gesendet: Donnerstag, 28. November 2019 09:18:30
>> Betreff: Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock
> 
>> On 28/11/2019 08:06, Dan Carpenter wrote:
>>> On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote:
>>>> A spin lock is taken here so we should use GFP_ATOMIC.
>>>>
>>>> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
>>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>>> ---
>>>>    arch/um/drivers/vector_kern.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
>>>> index 92617e16829e..6ff0065a271d 100644
>>>> --- a/arch/um/drivers/vector_kern.c
>>>> +++ b/arch/um/drivers/vector_kern.c
>>>> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device
>>>> *dev,
>>>>    		kfree(vp->bpf->filter);
>>>>    		vp->bpf->filter = NULL;
>>>>    	} else {
>>>> -		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL);
>>>> +		vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC);
>>>>    		if (vp->bpf == NULL) {
>>>>    			netdev_err(dev, "failed to allocate memory for firmware\n");
>>>>    			goto flash_fail;
>>>> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device
>>>> *dev,
>>>>    	if (request_firmware(&fw, efl->data, &vdevice->pdev.dev))
>>>               ^^^^^^^^^^^^^^^^
>>>
>>> Is it really possible to call request_firmware() while holding a
>>> spin_lock?  I was so sure that read from the disk.
>>
>> Works, I tested the patch quite a few times.
> 
> It works because of the nature of UML ->no  SMP or PREEMPT.
> But better request the firmware before taking the spinlock.
> request_firmware() can block.
> Same for the kmalloc(), just allocate the buffer before and then assign
> the pointer under the lock. That way you don't need GFP_ATOMIC.

Ack.

I will make an incremental on top of the existing patch (as that is 
already in -next

Brgds,

> 
> Thanks,
> //richard
> 
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
> 

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/