On Fri, May 26, 2023 at 10:33:21AM +0200, George Valkov wrote: > > > 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. Thanks, that looks good to me. > >> 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? Yes, I think that sounds good. Please wait 24h before the posting of v2 before posting v3, to allow time for more review of v3 (from others). ...
