From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0C3E9C4332B for ; Sun, 22 Mar 2020 16:53:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BD6492072E for ; Sun, 22 Mar 2020 16:53:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1584896004; bh=PoM6WPokFPax4XCKK0j4sFwE6uYh1cd7prgIs8gJuz0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=Z/F13gpEqx0eCUgyckrRLs2WELdOJXoHOQsNm2ZIVARNZ9U0d90HwJIN4h/9R30WI y9ekaXu3Ku+Dn3a0Nco40/60TSze4w29js7aJusb0EeZGIo2lb4k+WKrySu4K9iT5Z 1ZaWc0Q9Fp2LcU7bM3Ysqsa2G6jrLSUQerzc9PYg= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725971AbgCVQxY (ORCPT ); Sun, 22 Mar 2020 12:53:24 -0400 Received: from mail.kernel.org ([198.145.29.99]:41044 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725881AbgCVQxY (ORCPT ); Sun, 22 Mar 2020 12:53:24 -0400 Received: from archlinux (cpc149474-cmbg20-2-0-cust94.5-4.cable.virginm.net [82.4.196.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id E6EE2206F8; Sun, 22 Mar 2020 16:53:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1584896003; bh=PoM6WPokFPax4XCKK0j4sFwE6uYh1cd7prgIs8gJuz0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=EKzsfMaq4kI6h0cRWXjPeZaiC9IxTdKZwOalO+Pla+90hFDampCEwfoZExTAtPehS G2bswVGJVNjn7Vux+ArgdxMeeW1v4fin1cUhArq9QaGDxsI3pnzKMZYdGN6q9Xr+Jc n0ssDvKZuhKgUaLkCJmMiyVT0JUDf4DkudawaMNM= Date: Sun, 22 Mar 2020 16:53:17 +0000 From: Jonathan Cameron To: Kees Cook Cc: Andy Shevchenko , "Ardelean, Alexandru" , "lars@metafoo.de" , "linux-iio@vger.kernel.org" , "Grozav, Andrei" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Hennerich, Michael" , "Nagy, Laszlo" , "Csomortani, Istvan" , "robh+dt@kernel.org" , "Bogdan, Dragos" , "Costina, Adrian" Subject: Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core Message-ID: <20200322165317.0b1f0674@archlinux> In-Reply-To: <202003220901.880A6DF@keescook> References: <20200321085315.11030-1-alexandru.ardelean@analog.com> <20200321085315.11030-6-alexandru.ardelean@analog.com> <979ef870a4f0935e41e95e7759847eba8bd0407c.camel@analog.com> <202003220901.880A6DF@keescook> X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-iio-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org On Sun, 22 Mar 2020 09:16:36 -0700 Kees Cook 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 > > wrote: > > > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote: > > > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean > > > > 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? >