Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210219085826.46622-1-alexandru.ardelean@analog.com>
 <20210219085826.46622-2-alexandru.ardelean@analog.com> <20210222160157.0000391e@Huawei.com>
In-Reply-To: <20210222160157.0000391e@Huawei.com>
From:   Alexandru Ardelean <ardeleanalex@gmail.com>
Date:   Tue, 23 Feb 2021 08:36:15 +0200
Message-ID: <CA+U=DsppzH8si6Jd3PryeN=EingGcsRAibE_7ayGA+V-d7e-0g@mail.gmail.com>
Subject: Re: [PATCH 1/2] iio: core: use kfree_const in iio_free_chan_devattr_list()
 to free names
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc:     Alexandru Ardelean <alexandru.ardelean@analog.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        "Hennerich, Michael" <Michael.Hennerich@analog.com>,
        Jonathan Cameron <jic23@kernel.org>,
        =?UTF-8?B?TnVubyBTw6E=?= <nuno.sa@analog.com>,
        "Bogdan, Dragos" <dragos.bogdan@analog.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Feb 22, 2021 at 6:06 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Fri, 19 Feb 2021 10:58:25 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > When the buffer attributes were wrapped in iio_dev_attr types, I forgot to
> > duplicate the names, so that when iio_free_chan_devattr_list() gets called
> > on cleanup, these get free'd.
> > I stumbled over this while accidentally breaking a driver doing
> > iio_device_register(), and then the issue appeared.
> >
> > The fix can be just
> > 1. Just use kstrdup() during iio_buffer_wrap_attr()
> > 2. Just use kfree_const() during iio_free_chan_devattr_list
> > 3. Use both kstrdup_const() & kfree_const() (in the places mentioned above)
> >
> > Using kfree_const() should be sufficient, as the attribute names will
> > either be allocated or be stored in rodata.
> >
> > Fixes: a1a11142f66c ("iio: buffer: wrap all buffer attributes into iio_dev_attr")
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Thinking more on this...  It's fine for the users today, but there is
> nothing stopping a driver passing in names it allocated on the heap.  So
> I think we should revisit this.  Perhaps we need 1 or 3.

Ok.
Will re-send this as 3; that sounds like it gives the best of both worlds.

> > ---
> >  drivers/iio/industrialio-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 0d8c6e88d993..cb2735d2ae4b 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1358,7 +1358,7 @@ void iio_free_chan_devattr_list(struct list_head *attr_list)
> >       struct iio_dev_attr *p, *n;
> >
> >       list_for_each_entry_safe(p, n, attr_list, l) {
> > -             kfree(p->dev_attr.attr.name);
> > +             kfree_const(p->dev_attr.attr.name);
> >               list_del(&p->l);
> >               kfree(p);
> >       }
>