Re: [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv()

["Bence Csókás" <bence98@sch.bme.hu>]

All right, if you want to do that, fine. I did it this way, because it
took 1 less `goto`s... But your approach is equally valid.

But, to keep it consistent, if you change it one place, change it
everywhere else.

Regards,
Bence

Dan Carpenter <dan.carpenter@oracle.com> ezt írta (időpont: 2021. máj.
12., Sze, 15:11):
>
> On Wed, May 12, 2021 at 12:54:47PM +0200, Bence Csókás wrote:
> > Dan Carpenter <dan.carpenter@oracle.com> ezt írta (időpont: 2021. máj.
> > 12., Sze, 12:07):
> > >
> > > We need to add a check for if the kzalloc() fails.
> >
> > That is correct, I missed that :/
> >
> > >
> > > Fixes: 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > >  drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++----
> > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
> > > index 78cfecd1ea76..3ded28632e4c 100644
> > > --- a/drivers/i2c/busses/i2c-cp2615.c
> > > +++ b/drivers/i2c/busses/i2c-cp2615.c
> > > @@ -138,17 +138,23 @@ cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
> > >  static int
> > >  cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
> > >  {
> > > -       struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
> > > -       struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;
> >
> > These two lines could stay as-is, since the invalid i2c_r will never be used.
> >
>
> Yeah, I know but people shouldn't put functions which can fail inside
> the declaration block.  And I also know that doing:
>
>         struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;
>
> is fine even when "msg" is NULL.  But don't do that, because it is ugly.
>
> > >         struct usb_device *usbdev = interface_to_usbdev(usbif);
> > > -       int res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN),
> > > -                              msg, sizeof(struct cp2615_iop_msg), NULL, 0);
> > > +       struct cp2615_iop_msg *msg;
> > > +       struct cp2615_i2c_transfer_result *i2c_r;
> > > +       int res;
> > > +
> > > +       msg = kzalloc(sizeof(*msg), GFP_KERNEL);
> > > +       if (!msg)
> > > +               return -ENOMEM;
> >
> > You will want to also make `cp2615_init_iop_msg()` return -ENOMEM
> > instead of -EINVAL, for consistency's sake.
> >
>
> I view that as unrelated and I'm sorry but this error handling code is
> not really best practices...  I don't have time to fix it up.  :/
>
> drivers/i2c/busses/i2c-cp2615.c
>    124  static int
>    125  cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
>    126  {
>    127          struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
>
> Don't put functions which can fail inside the declaration block.
> Although, it is relatively uncommon to put allocations inside the
> declaration block these sorts of allocations are over represented when
> it comes to memory leaks and other static analysis bugs.
>
>    128          struct usb_device *usbdev = interface_to_usbdev(usbif);
>    129          int res = cp2615_init_i2c_msg(msg, i2c_w);
>
> Handle failures immediately.  Try not to mix the error paths and the
> success paths.  After a function call, then you're going to have to deal
> with both failure and success path, but the failure path is just
> "clean up and return an error code" and the success path is hopefully
> going to continue for days or months.  So get the shorter failure path
> out of the way first, then continue with the success path.
>
>    130
>    131          if (!res)
>
> Always do error handling, don't do success handling.
>
>    132                  res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT),
>    133                                     msg, ntohs(msg->length), NULL, 0);
>
> The success path should be indented one tab and the error handling
> should be indented two tabs.
>
>    134          kfree(msg);
>    135          return res;
>    136  }
>
> The function should look like this.
>
> static int
> cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
> {
>         struct usb_device *usbdev = interface_to_usbdev(usbif);
>         struct cp2615_iop_msg *msg;
>         int res;
>
>         msg = kzalloc(sizeof(*msg), GFP_KERNEL);
>         if (!msg)
>                 return -ENOMEM;
>
>         res = cp2615_init_i2c_msg(msg, i2c_w);
>         if (res)
>                 goto free;
>
>         res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT),
>                            msg, ntohs(msg->length), NULL, 0);
> free:
>         kfree(msg);
>         return res;
> }
>
> regards,
> dan carpenter
>