> On 26 May 2023, at 10:52 AM, Simon Horman <simon.horman@xxxxxxxxxxxx> wrote: > > On Thu, May 25, 2023 at 09:42:54PM +0200, Foster Snowhill wrote: >> From: Georgi Valkov <gvalkov@xxxxxxxxx> >> >> The cleanup precedure in ipheth_probe will attempt to free a >> NULL pointer in dev->ctrl_buf if the memory allocation for >> this buffer is not successful. Rearrange the goto labels to >> avoid this risk. > > Hi Georgi and Foster, > > kfree will ignore a NULL argument, so I think the existing code is safe. > But given the name of the label I do agree there is scope for a cleanup > here. It’s good to know that precaution has been taken in kfree to avoid this, yet at my opinion knowingly attempting to free a NULL pointer is a red flag and bad design. Likely a misplaced label. > Could you consider rewording the patch description accordingly? What would you like me to use as title and description? Can I use this? usbnet: ipheth: avoid kfree with a NULL pointer The cleanup precedure in ipheth_probe will attempt to free a NULL pointer in dev->ctrl_buf if the memory allocation for this buffer is not successful. While kfree ignores NULL pointers, and the existing code is safe, it is a better design to rearrange the goto labels and avoid this. >> Signed-off-by: Georgi Valkov <gvalkov@xxxxxxxxx> > > If Georgi is the author of the patch, which seems to be the case, > then the above is correct. But as the patch is being posted by Foster > I think it should be followed by a Signed-off-by line for Foster. Yes, I discovered the potential issue and authored the patch to help. We’ll append Signed-off-by Foster as you suggested. Thanks Simon! Something like that?
Attachment:
0001-usbnet-ipheth-avoid-kfree-with-a-NULL-pointer.patch
Description: Binary data
Georgi Valkov httpstorm.com nano RTOS > Link: https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed+off#developer-s-certificate-of-origin-1-1 > >> --- >> drivers/net/usb/ipheth.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c >> index 6a769df0b..8875a3d0e 100644 >> --- a/drivers/net/usb/ipheth.c >> +++ b/drivers/net/usb/ipheth.c >> @@ -510,8 +510,8 @@ static int ipheth_probe(struct usb_interface *intf, >> ipheth_free_urbs(dev); >> err_alloc_urbs: >> err_get_macaddr: >> -err_alloc_ctrl_buf: >> kfree(dev->ctrl_buf); >> +err_alloc_ctrl_buf: >> err_endpoints: >> free_netdev(netdev); >> return retval; > > -- > pw-bot: cr
