linux-kernel - Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 18 Oct 2020 10:56:31 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Lino Sanfilippo <LinoSanfilippo@....de>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of
 error

On Fri, 16 Oct 2020 18:09:48 +0300
Andy Shevchenko <andy.shevchenko@...il.com> wrote:

> On Sun, Oct 11, 2020 at 9:24 PM Lino Sanfilippo <LinoSanfilippo@....de> wrote:
> >
> > In function map_array_register() properly rewind in case of error.
> > Furthermore remove the now superfluous initialization of "ret" with 0.  
> 
> >  int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> >  {
> > -       int i = 0, ret = 0;
> > +       int i = 0, ret;
> >         struct iio_map_internal *mapi;
> >
> >         if (maps == NULL)
> > @@ -44,7 +44,18 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> >                 list_add_tail(&mapi->l, &iio_map_list);
> >                 i++;
> >         }
> > +       mutex_unlock(&iio_map_list_lock);
> > +
> > +       return 0;
> > +
> >  error_ret:  
> 
> Wait a bit.
> First of all we linked all successfully added items to the list.
> From this we have two ways to go:
>  - leave with as many maps as we registered
>  - clean up and bail out
> 
> I dunno which one would play better in IIO, but you seem to go with
> the latter one.

Better to cleanup and bail out I think.   It's fairly unlikely
a consumer is going to be ready to cope with getting a partial
set of the channels it's expecting.


> 
> > +       /* undo */
> > +       while (i--) {
> > +               mapi = list_last_entry(&iio_map_list, struct iio_map_internal,
> > +                                      l);
> > +               list_del(&mapi->l);
> > +               kfree(mapi);
> > +       }  
> 
> We have iio_map_array_unregister(). Why not use it?
> 
> >         mutex_unlock(&iio_map_list_lock);  
> 
> I would rather drop a label with replacement goto -> break inside the
> loop and call the following

I argued for the goto, but it is indeed less obviously the right
thing to do once we are using iio_map_array_unregister.

> 
> 
>         mutex_unlock(&iio_map_list_lock);
> if (ret)
>   iio_map_array_unregister();
> return ret;
> 
> Sounds like only a few LOCs are needed.
>