| Message ID | 20230527130309.34090-1-forst@pen.gy (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3,1/2] usbnet: ipheth: fix risk of NULL pointer deallocation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Single patches do not need cover letters |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Sat, May 27, 2023 at 03:03:08PM +0200, Foster Snowhill wrote: > From: Georgi Valkov <gvalkov@gmail.com> > > 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@gmail.com> > Signed-off-by: Foster Snowhill <forst@pen.gy> Reviewed-by: Simon Horman <simon.horman@corigine.com>
Hi, On Sat, 2023-05-27 at 15:03 +0200, Foster Snowhill wrote: > From: Georgi Valkov <gvalkov@gmail.com> > > 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@gmail.com> > Signed-off-by: Foster Snowhill <forst@pen.gy> If you are going to repost (due to changes in patch 2) please update this patch subj, too. Currently is a bit confusing, something alike "cleanup the initialization error path" would be more clear. Thanks, Paolo
Thanks Paolo! Something like that? Georgi Valkov httpstorm.com nano RTOS > On 30 May 2023, at 2:02 PM, Paolo Abeni <pabeni@redhat.com> wrote: > > Hi, > > On Sat, 2023-05-27 at 15:03 +0200, Foster Snowhill wrote: >> From: Georgi Valkov <gvalkov@gmail.com> >> >> 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@gmail.com> >> Signed-off-by: Foster Snowhill <forst@pen.gy> > > If you are going to repost (due to changes in patch 2) please update > this patch subj, too. Currently is a bit confusing, something alike > "cleanup the initialization error path" would be more clear. > > Thanks, > > Paolo >
Hi Paolo! Sorry, I attached the old version by mistake. Here is the new version: Georgi Valkov httpstorm.com nano RTOS > On 30 May 2023, at 2:02 PM, Paolo Abeni <pabeni@redhat.com> wrote: > > Hi, > > On Sat, 2023-05-27 at 15:03 +0200, Foster Snowhill wrote: >> From: Georgi Valkov <gvalkov@gmail.com> >> >> 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@gmail.com> >> Signed-off-by: Foster Snowhill <forst@pen.gy> > > If you are going to repost (due to changes in patch 2) please update > this patch subj, too. Currently is a bit confusing, something alike > "cleanup the initialization error path" would be more clear. > > Thanks, > > Paolo >
On Tue, 2023-05-30 at 14:11 +0300, George Valkov wrote:
> Sorry, I attached the old version by mistake. Here is the new version:
LGTM.
Please in future prefer inline patch vs attachments even for
discussion.
Note that you will have to formally repost both patches.
Thanks!
Paolo
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;