Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Jonathan Cameron]

On Sun, 22 Mar 2020 09:16:36 -0700
Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx/

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?
>