Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Kees Cook <keescook@chromium.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>,
	"Ardelean, Alexandru" <alexandru.Ardelean@analog.com>,
	"lars@metafoo.de" <lars@metafoo.de>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	"Grozav, Andrei" <Andrei.Grozav@analog.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Hennerich, Michael" <Michael.Hennerich@analog.com>,
	"Nagy, Laszlo" <Laszlo.Nagy@analog.com>,
	"Csomortani, Istvan" <Istvan.Csomortani@analog.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"Bogdan, Dragos" <Dragos.Bogdan@analog.com>,
	"Costina, Adrian" <Adrian.Costina@analog.com>
Subject: Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core
Date: Sun, 22 Mar 2020 16:53:17 +0000
Message-ID: <20200322165317.0b1f0674@archlinux> (raw)
In-Reply-To: <202003220901.880A6DF@keescook>

On Sun, 22 Mar 2020 09:16:36 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Sun, Mar 22, 2020 at 12:45:39PM +0200, Andy Shevchenko wrote:
> > +Cc Kees (see below about allocation size checks)
> > 
> > On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> > <alexandru.Ardelean@analog.com> wrote:  
> > > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:  
> > > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > > <alexandru.ardelean@analog.com> wrote:  
> > 
> > ...
> >   
> > > > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device
> > > > > *dev,
> > > > > +                                                         int sizeof_priv)
> > > > > +{
> > > > > +       struct adi_axi_adc_client *cl;
> > > > > +       size_t alloc_size;
> > > > > +
> > > > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > > > +       if (sizeof_priv) {
> > > > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > +               alloc_size += sizeof_priv;
> > > > > +       }
> > > > > +       alloc_size += IIO_ALIGN - 1;  
> > > >
> > > > Have you looked at linux/overflow.h?  
> > >
> > > i did now;
> > > any hints where i should look closer?  
> > 
> > It seems it lacks of this kind of allocation size checks... Perhaps add one?
> > Kees, what do you think?
> >   
> > > > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > > +       if (!cl)
> > > > > +               return ERR_PTR(-ENOMEM);  
> 
> My head hurts trying to read this! ;) Okay, so the base size is
> sizeof(struct adi_axi_adc_client). But if sizeof_priv is non-zero
> (this arg should be size_t not int), then we need to make the struct
> size ALIGNed? And then what is the "+= IIO_ALIGN - 1" for?

I'm a bit embarrassed.  I can't remember what the += IIO_ALIGN - 1
was for in the first place and I can't work it out now.

The purpose of the fun here was to end up with a structure that
was either
a) sizeof(struct iio_dev) long,
b) sizeof(struct iio_dev) + padding + sizeof_priv 
where the padding ensured that any __cacheline_aligned elements
in the private structure were cacheline aligned within resulting
allocation.

So why the extra IIO_ALIGN - 1....

The original patch doesn't help much either given it's got a question
in there for why this bit is needed.

https://lore.kernel.org/linux-iio/1302890160-8823-5-git-send-email-jic23@cam.ac.uk/

However, it rang a slight bell.  Seems I lifted the code from netdev.
https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L9718

I'm fairly sure we don't need that padding here..  What can I say,
I was young and stupid :)

I did add a question mark so clearly meant to come back and
take another look ;)

One vague thought is that it's about ensuring we are big enough to
ensure we are cacheline aligned.  That's obviously not a problem with
current struct iio_dev which is far from small,
but in theory it could have been.  Also, thinking about it we only
need the struct iio_dev to be cacheline aligned if we have
an iio_priv structure.  If we have one of those it will definitely
be big enough anyway.

At somepoint I'd like to look at cleaning it up for iio_device_alloc
but with a lot of testing as who knows what is relying on this behaviour
or if I've missed something.  Crashes around this alignment are
infrequent and nasty to trace at the best of times.

Jonathan

> 
> It's not clear to me what the expect alignment/padding is here.
> 
> I would probably construct this as:
> 
> 	sizeof_self = sizeof(struct adi_axi_adc_client);
> 	if (sizeof_priv)
> 		sizeof_self = ALIGN(sizeof_self, IIO_ALIGN);
> 	if (check_add_overflow(sizeof_self, sizeof_priv, &sizeof_alloc))
> 		return ERR_PTR(-ENOMEM);
> 	if (check_add_overflow(sizeof_alloc, IIO_ALIGN - 1, &sizeof_alloc))
> 		return ERR_PTR(-ENOMEM);
> 
> But I don't understand the "IIO_ALIGN - 1" part, so I assume this could
> be shortened with better use of ALIGN()?
> 
> Also, this feels like a weird driver allocation overall:
> 
> +	struct adi_axi_adc_conv **ptr, *conv;
> +
> +	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> 
> devres_alloc() allocates storage for a _single pointer_. :P That's not
> useful for resource tracking. Why is devres_alloc() being called here
> and not down in adi_axi_adc_conv_register() and just passing the pointer
> back up?
> 


  parent reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-21  8:53 [PATCH v11 0/8] iio: adi-axi-adc,ad9647: Add support for AD9467 ADC Alexandru Ardelean
2020-03-21  8:53 ` [PATCH v11 1/8] include: fpga: adi-axi-common.h: fixup whitespace tab -> space Alexandru Ardelean
2020-03-21  8:53 ` [PATCH v11 2/8] include: fpga: adi-axi-common.h: add version helper macros Alexandru Ardelean
2020-03-21 17:21   ` Jonathan Cameron
2020-03-21  8:53 ` [PATCH v11 3/8] iio: buffer-dmaengine: use %zu specifier for sprintf(align) Alexandru Ardelean
2020-03-21 17:22   ` Jonathan Cameron
2020-03-21  8:53 ` [PATCH v11 4/8] iio: buffer-dmaengine: add dev-managed calls for buffer alloc Alexandru Ardelean
2020-03-21 17:22   ` Jonathan Cameron
2020-03-21  8:53 ` [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core Alexandru Ardelean
2020-03-21 17:15   ` Jonathan Cameron
2020-03-21 21:38   ` Andy Shevchenko
2020-03-22  9:35     ` Ardelean, Alexandru
2020-03-22 10:45       ` Andy Shevchenko
2020-03-22 16:11         ` Ardelean, Alexandru
2020-03-22 16:16         ` Kees Cook
2020-03-22 16:31           ` Ardelean, Alexandru
2020-03-22 16:44             ` Ardelean, Alexandru
2020-03-22 16:53           ` Jonathan Cameron [this message]
2020-03-22 17:40             ` Ardelean, Alexandru
2020-03-22 18:26               ` Jonathan Cameron
2020-03-24  7:10                 ` Ardelean, Alexandru
2020-03-22 15:20       ` Jonathan Cameron
2020-03-21  8:53 ` [PATCH v11 6/8] dt-bindings: iio: adc: add bindings doc for AXI ADC driver Alexandru Ardelean
2020-03-21 17:23   ` Jonathan Cameron
2020-03-21  8:53 ` [PATCH v11 7/8] iio: adc: ad9467: add support AD9467 ADC Alexandru Ardelean
2020-03-21 17:23   ` Jonathan Cameron
2020-03-21  8:53 ` [PATCH v11 8/8] dt-bindings: iio: adc: add bindings doc for " Alexandru Ardelean
2020-03-21 17:24   ` Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200322165317.0b1f0674@archlinux \
    --to=jic23@kernel.org \
    --cc=Adrian.Costina@analog.com \
    --cc=Andrei.Grozav@analog.com \
    --cc=Dragos.Bogdan@analog.com \
    --cc=Istvan.Csomortani@analog.com \
    --cc=Laszlo.Nagy@analog.com \
    --cc=Michael.Hennerich@analog.com \
    --cc=alexandru.Ardelean@analog.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=keescook@chromium.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git