diff mbox series

[v4,1/1] test_fimware: return -ENOMEM instead of -ENOSPC on failed memory allocation

Message ID 20230812054346.168223-1-mirsad.todorovac@alu.unizg.hr
State New
Headers
Series [v4,1/1] test_fimware: return -ENOMEM instead of -ENOSPC on failed memory allocation |

Commit Message

Mirsad Todorovac Aug. 12, 2023, 5:43 a.m. UTC 
  [ Upstream commit 7dae593cd226a0bca61201cf85ceb9335cf63682 ]

In a couple of situations like

	name = kstrndup(buf, count, GFP_KERNEL);
	if (!name)
		return -ENOSPC;

the error is not actually "No space left on device", but "Out of memory".

It is semantically correct to return -ENOMEM in all failed kstrndup()
and kzalloc() cases in this driver, as it is not a problem with disk
space, but with kernel memory allocator failing allocation.

The semantically correct should be:

        name = kstrndup(buf, count, GFP_KERNEL);
        if (!name)
                return -ENOMEM;

Cc: Dan Carpenter <error27@gmail.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: stable@vger.kernel.org # 4.14
Fixes: c92316bf8e948 ("test_firmware: add batched firmware tests")
Fixes: 0a8adf584759c ("test: add firmware_class loader test")
Fixes: eb910947c82f9 ("test: firmware_class: add asynchronous request trigger")
Fixes: 061132d2b9c95 ("test_firmware: add test custom fallback trigger")
Link: https://lore.kernel.org/all/20230606070808.9300-1-mirsad.todorovac@alu.unizg.hr/
Signed-off-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>

[ This is the backport of the patch to 4.19 and 4.14 branches. There are no	]
[ semantic differences in the commit. Backport is provided for completenes sake	]
[ so it would apply to all of the supported LTS kernels				]

---
v3 -> v4:
 no changes. resubmitting for 4.14 because the patchwork didn't apply to the 4.14 tree.

v2 -> v3:
 minor clarifications with the versioning for the patchwork. no change to commit.

v1 -> v2:
 removed the Reviewed-by: and Acked-by tags, as this is a slightly different patch and
 those need to be reacquired

 lib/test_firmware.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Greg KH Aug. 12, 2023, 7:29 a.m. UTC | #1 
On Sat, Aug 12, 2023 at 07:43:47AM +0200, Mirsad Todorovac wrote:
> [ Upstream commit 7dae593cd226a0bca61201cf85ceb9335cf63682 ]
> 
> In a couple of situations like
> 
> 	name = kstrndup(buf, count, GFP_KERNEL);
> 	if (!name)
> 		return -ENOSPC;
> 
> the error is not actually "No space left on device", but "Out of memory".
> 
> It is semantically correct to return -ENOMEM in all failed kstrndup()
> and kzalloc() cases in this driver, as it is not a problem with disk
> space, but with kernel memory allocator failing allocation.
> 
> The semantically correct should be:
> 
>         name = kstrndup(buf, count, GFP_KERNEL);
>         if (!name)
>                 return -ENOMEM;
> 
> Cc: Dan Carpenter <error27@gmail.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: stable@vger.kernel.org # 4.14
> Fixes: c92316bf8e948 ("test_firmware: add batched firmware tests")
> Fixes: 0a8adf584759c ("test: add firmware_class loader test")
> Fixes: eb910947c82f9 ("test: firmware_class: add asynchronous request trigger")
> Fixes: 061132d2b9c95 ("test_firmware: add test custom fallback trigger")
> Link: https://lore.kernel.org/all/20230606070808.9300-1-mirsad.todorovac@alu.unizg.hr/
> Signed-off-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> 
> [ This is the backport of the patch to 4.19 and 4.14 branches. There are no	]
> [ semantic differences in the commit. Backport is provided for completenes sake	]
> [ so it would apply to all of the supported LTS kernels				]

This commit is already in the 4.19.291 release, does it need to be
included in there again for some reason?

thanks,

greg k-h
Mirsad Todorovac Aug. 12, 2023, 8:06 a.m. UTC | #2 
On 8/12/23 09:29, Greg KH wrote:
> On Sat, Aug 12, 2023 at 07:43:47AM +0200, Mirsad Todorovac wrote:
>> [ Upstream commit 7dae593cd226a0bca61201cf85ceb9335cf63682 ]
>>
>> In a couple of situations like
>>
>> 	name = kstrndup(buf, count, GFP_KERNEL);
>> 	if (!name)
>> 		return -ENOSPC;
>>
>> the error is not actually "No space left on device", but "Out of memory".
>>
>> It is semantically correct to return -ENOMEM in all failed kstrndup()
>> and kzalloc() cases in this driver, as it is not a problem with disk
>> space, but with kernel memory allocator failing allocation.
>>
>> The semantically correct should be:
>>
>>          name = kstrndup(buf, count, GFP_KERNEL);
>>          if (!name)
>>                  return -ENOMEM;
>>
>> Cc: Dan Carpenter <error27@gmail.com>
>> Cc: Takashi Iwai <tiwai@suse.de>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
>> Cc: Brian Norris <computersforpeace@gmail.com>
>> Cc: stable@vger.kernel.org # 4.14
>> Fixes: c92316bf8e948 ("test_firmware: add batched firmware tests")
>> Fixes: 0a8adf584759c ("test: add firmware_class loader test")
>> Fixes: eb910947c82f9 ("test: firmware_class: add asynchronous request trigger")
>> Fixes: 061132d2b9c95 ("test_firmware: add test custom fallback trigger")
>> Link: https://lore.kernel.org/all/20230606070808.9300-1-mirsad.todorovac@alu.unizg.hr/
>> Signed-off-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
>>
>> [ This is the backport of the patch to 4.19 and 4.14 branches. There are no	]
>> [ semantic differences in the commit. Backport is provided for completenes sake	]
>> [ so it would apply to all of the supported LTS kernels				]
> 
> This commit is already in the 4.19.291 release, does it need to be
> included in there again for some reason?

Hi Mr. Greg,

I think the patchwork did not apply the commit to the 4.14 stable tree.
Only the 19be3eccd000 ("test_firmware: fix a memory leak with reqs buffer" propagated to 4.14.322.

I would like to have us this chapter (backporting) completed before moving on.

Kind regards,
Mirsad Todorovac
Greg KH Aug. 12, 2023, 8:12 a.m. UTC | #3 
On Sat, Aug 12, 2023 at 10:06:53AM +0200, Mirsad Todorovac wrote:
> 
> 
> On 8/12/23 09:29, Greg KH wrote:
> > On Sat, Aug 12, 2023 at 07:43:47AM +0200, Mirsad Todorovac wrote:
> > > [ Upstream commit 7dae593cd226a0bca61201cf85ceb9335cf63682 ]
> > > 
> > > In a couple of situations like
> > > 
> > > 	name = kstrndup(buf, count, GFP_KERNEL);
> > > 	if (!name)
> > > 		return -ENOSPC;
> > > 
> > > the error is not actually "No space left on device", but "Out of memory".
> > > 
> > > It is semantically correct to return -ENOMEM in all failed kstrndup()
> > > and kzalloc() cases in this driver, as it is not a problem with disk
> > > space, but with kernel memory allocator failing allocation.
> > > 
> > > The semantically correct should be:
> > > 
> > >          name = kstrndup(buf, count, GFP_KERNEL);
> > >          if (!name)
> > >                  return -ENOMEM;
> > > 
> > > Cc: Dan Carpenter <error27@gmail.com>
> > > Cc: Takashi Iwai <tiwai@suse.de>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> > > Cc: Brian Norris <computersforpeace@gmail.com>
> > > Cc: stable@vger.kernel.org # 4.14
> > > Fixes: c92316bf8e948 ("test_firmware: add batched firmware tests")
> > > Fixes: 0a8adf584759c ("test: add firmware_class loader test")
> > > Fixes: eb910947c82f9 ("test: firmware_class: add asynchronous request trigger")
> > > Fixes: 061132d2b9c95 ("test_firmware: add test custom fallback trigger")
> > > Link: https://lore.kernel.org/all/20230606070808.9300-1-mirsad.todorovac@alu.unizg.hr/
> > > Signed-off-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> > > 
> > > [ This is the backport of the patch to 4.19 and 4.14 branches. There are no	]
> > > [ semantic differences in the commit. Backport is provided for completenes sake	]
> > > [ so it would apply to all of the supported LTS kernels				]
> > 
> > This commit is already in the 4.19.291 release, does it need to be
> > included in there again for some reason?
> 
> Hi Mr. Greg,
> 
> I think the patchwork did not apply the commit to the 4.14 stable tree.
> Only the 19be3eccd000 ("test_firmware: fix a memory leak with reqs buffer" propagated to 4.14.322.
> 
> I would like to have us this chapter (backporting) completed before moving on.

That's fine, but your comment here said that this is for 4.19, but this
is already in 4.19, so I'm confused.  I'll queue this up for 4.14.y
now...

thanks,

greg k-h
Mirsad Todorovac Aug. 12, 2023, 8:53 a.m. UTC | #4 
On 8/12/23 10:12, Greg KH wrote:
> On Sat, Aug 12, 2023 at 10:06:53AM +0200, Mirsad Todorovac wrote:
>>
>>
>> On 8/12/23 09:29, Greg KH wrote:
>>> On Sat, Aug 12, 2023 at 07:43:47AM +0200, Mirsad Todorovac wrote:
>>>> [ Upstream commit 7dae593cd226a0bca61201cf85ceb9335cf63682 ]
>>>>
>>>> In a couple of situations like
>>>>
>>>> 	name = kstrndup(buf, count, GFP_KERNEL);
>>>> 	if (!name)
>>>> 		return -ENOSPC;
>>>>
>>>> the error is not actually "No space left on device", but "Out of memory".
>>>>
>>>> It is semantically correct to return -ENOMEM in all failed kstrndup()
>>>> and kzalloc() cases in this driver, as it is not a problem with disk
>>>> space, but with kernel memory allocator failing allocation.
>>>>
>>>> The semantically correct should be:
>>>>
>>>>           name = kstrndup(buf, count, GFP_KERNEL);
>>>>           if (!name)
>>>>                   return -ENOMEM;
>>>>
>>>> Cc: Dan Carpenter <error27@gmail.com>
>>>> Cc: Takashi Iwai <tiwai@suse.de>
>>>> Cc: Kees Cook <keescook@chromium.org>
>>>> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
>>>> Cc: Brian Norris <computersforpeace@gmail.com>
>>>> Cc: stable@vger.kernel.org # 4.14
>>>> Fixes: c92316bf8e948 ("test_firmware: add batched firmware tests")
>>>> Fixes: 0a8adf584759c ("test: add firmware_class loader test")
>>>> Fixes: eb910947c82f9 ("test: firmware_class: add asynchronous request trigger")
>>>> Fixes: 061132d2b9c95 ("test_firmware: add test custom fallback trigger")
>>>> Link: https://lore.kernel.org/all/20230606070808.9300-1-mirsad.todorovac@alu.unizg.hr/
>>>> Signed-off-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
>>>>
>>>> [ This is the backport of the patch to 4.19 and 4.14 branches. There are no	]
>>>> [ semantic differences in the commit. Backport is provided for completenes sake	]
>>>> [ so it would apply to all of the supported LTS kernels				]
>>>
>>> This commit is already in the 4.19.291 release, does it need to be
>>> included in there again for some reason?
>>
>> Hi Mr. Greg,
>>
>> I think the patchwork did not apply the commit to the 4.14 stable tree.
>> Only the 19be3eccd000 ("test_firmware: fix a memory leak with reqs buffer" propagated to 4.14.322.
>>
>> I would like to have us this chapter (backporting) completed before moving on.
> 
> That's fine, but your comment here said that this is for 4.19, but this
> is already in 4.19, so I'm confused.  I'll queue this up for 4.14.y
> now...

I will not contradict, however, it is the verbatim same patch as for the 4.19 stable tree.

I know that you have to quick pick among hundreds of patches each day, because I follow the commit log.
So I try to guess what would be the best, but I am still kinda newbie to the patch submission process. :-)

I think we got it sorted out right this time.

I will cooperate with the required number of iterations, as I understood this is an incremental development.

However, please note that I could not boot the 4.14 kernel, it was only built properly.
There is no reason for this change to work, but the actual test is always preferred, of course.

Kind regards,
Mirsad Todorovac
diff mbox series

Patch

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 5318c5e18acf..34210306ea66 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -159,7 +159,7 @@  static int __kstrncpy(char **dst, const char *name, size_t count, gfp_t gfp)
 {
 	*dst = kstrndup(name, count, gfp);
 	if (!*dst)
-		return -ENOSPC;
+		return -ENOMEM;
 	return count;
 }
 
@@ -459,7 +459,7 @@  static ssize_t trigger_request_store(struct device *dev,
 
 	name = kstrndup(buf, count, GFP_KERNEL);
 	if (!name)
-		return -ENOSPC;
+		return -ENOMEM;
 
 	pr_info("loading '%s'\n", name);
 
@@ -500,7 +500,7 @@  static ssize_t trigger_async_request_store(struct device *dev,
 
 	name = kstrndup(buf, count, GFP_KERNEL);
 	if (!name)
-		return -ENOSPC;
+		return -ENOMEM;
 
 	pr_info("loading '%s'\n", name);
 
@@ -543,7 +543,7 @@  static ssize_t trigger_custom_fallback_store(struct device *dev,
 
 	name = kstrndup(buf, count, GFP_KERNEL);
 	if (!name)
-		return -ENOSPC;
+		return -ENOMEM;
 
 	pr_info("loading '%s' using custom fallback mechanism\n", name);