Message ID | 20231031210521.1661552-1-dlechner@baylibre.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:abcd:0:b0:403:3b70:6f57 with SMTP id f13csp22196vqx; Tue, 31 Oct 2023 14:06:11 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHSQBFSRe42NPmrhtG6Aqn31wZspdAWTngSPdWAZvHkOVV6eg84AX8zNHP8tZMdV6YFIJvz X-Received: by 2002:a17:902:eccd:b0:1cc:54b5:b4ff with SMTP id a13-20020a170902eccd00b001cc54b5b4ffmr5573486plh.54.1698786371336; Tue, 31 Oct 2023 14:06:11 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698786371; cv=none; d=google.com; s=arc-20160816; b=A+W48lS+LMuNxyfFb4ktkSOImyom79b58nhk+nUu8T5wL7YtjZ/BAa4tEDPFcqcMp1 ZVbtczVW9MHD9qkEeNJJkS4VxH75i8FxWSQ237+HJ6TG537nPLypqZsXLoswwAvydwEL 34y8juo6zPYY4yET/4jkXueV6oH0fp0hHBgsTwiz2nh2SXRSDCHVcZs5VV9smPHav/5j JxHknYBOWPPyXtC3mzcoGx7SXFC1bNS+T5g68DnbXHlaVXcyb6FUbqrSTpy9EBs+Orvn zm03YqDy7NifN4v4jSKqiW2fxvgzEfk66H1mscxBYzSa+LA/jKwjX2ReFE9nFpJBzvQw dJZg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=ylAWoXi+6yzfzvduSB8hqrTOh1zLBHDnHEqXnt/b0ys=; fh=VsrPHOvL5R6t/lQ7gnTA6Kjj6fFz7LygBNologixHRw=; b=qWRcgdJjzQY9bRMh2+DStEOQ3+B354oPMu3nAOMZL2ZJx1Top6Cf42iTpMa4EsoG4F EJ6HXyzy4VAXZay6CwdHS3Kj7/JcaFzMbKM2w2EFpuhwGQCfDgrFhLb9tjtuYEuAsYvJ 8AKTR8hi4EzxZhsiSbFHq7XCqVwSI5064LHhTX8ThBk3zt/RNOVYK7tzjLe77TXMIpK2 mwoYf+uGJ/H3w84N42blJs/7ufxwZre885hVZeWsZTpPdaf950zdiKIFC1Mq9XnR691E fUo3kpT4fm5lKRiggvRK9fTZTI3rBayUbOdUKFnaCdo0GdLB/ALNnWew8OC0P/fWWTmK PWBQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@baylibre-com.20230601.gappssmtp.com header.s=20230601 header.b=fgSUFs3m; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from pete.vger.email (pete.vger.email. [2620:137:e000::3:6]) by mx.google.com with ESMTPS id s5-20020a170903214500b001c75540d9fesi1447798ple.587.2023.10.31.14.06.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 Oct 2023 14:06:11 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6; Authentication-Results: mx.google.com; dkim=pass header.i=@baylibre-com.20230601.gappssmtp.com header.s=20230601 header.b=fgSUFs3m; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 95FE9807971E; Tue, 31 Oct 2023 14:06:04 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232059AbjJaVFs (ORCPT <rfc822;chrisjones.unixmen@gmail.com> + 33 others); Tue, 31 Oct 2023 17:05:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51820 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231921AbjJaVFr (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 31 Oct 2023 17:05:47 -0400 Received: from mail-oa1-x34.google.com (mail-oa1-x34.google.com [IPv6:2001:4860:4864:20::34]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1237AF3 for <linux-kernel@vger.kernel.org>; Tue, 31 Oct 2023 14:05:45 -0700 (PDT) Received: by mail-oa1-x34.google.com with SMTP id 586e51a60fabf-1e5bc692721so3742801fac.0 for <linux-kernel@vger.kernel.org>; Tue, 31 Oct 2023 14:05:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20230601.gappssmtp.com; s=20230601; t=1698786344; x=1699391144; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=ylAWoXi+6yzfzvduSB8hqrTOh1zLBHDnHEqXnt/b0ys=; b=fgSUFs3mbPlBsK5wCQNOa4ndbVCUacyPUo7OR7Lw8h5/PS4WWsckjB/1tLzsOGhp9p rgEJpOZ3UYRoIYEXC5ZI6Zvuh0J68s1KHm10rhdIpp/r0ZVP4Z0UhAU4DAezJ5SBkpux HRuEJmzHuJO3deWyeIqgDtD3fnGRrsN5wsXwbScu7HpFYWuf+KbXMylrk+MNl5KLeSWA yrH4gvqkBB7os5c1JjKRDIWf7io6rqvuAAnZD88Zw/hq+oNy0Iuq28Lv55QFH+bxuxPE LxV8DSNsS1WysAMkC6QeKCtkd6KdscjDcjTEtr7DdZiODzGF7Q19n7SjKP9gqXPR5H84 KFnA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698786344; x=1699391144; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ylAWoXi+6yzfzvduSB8hqrTOh1zLBHDnHEqXnt/b0ys=; b=vFSb24WkVecUZdINAIhIxI6aVPTv+4Fx0sh/74TqQNo+qmQl1DSgJj3hxJibVrXp2W hpaeQFSCiI/jF0Pp1Xn8um1i6L63I8ubZ5naJEvY5v0KIOl1BdouZ+QY0r1PwOALbVH6 WMYJWm0FavYc6ENHwrjEAyLjz2IAKGvZXKR51wjOjn9teanU3aarWwVcAocLdjTL8Txf dqDfNRSzHTgSfZL+aZWtTDiSvfWsBJbhypaM4l+mKMVc3RcrTpLwU7GlacndQ+veGYuz gLwRh2FdsO4Rox6fmQaFQ+tcHL/JKmk0VVv0HiXxXGEvw/RHui48oS5oo2cw5ZsCNNVF 2aqg== X-Gm-Message-State: AOJu0YyVL67VkfbChLhgVJGWagmPK1gT+d8GVSM0iDpfMOEBiOinjhi/ FguIvVdnXabtw74cw/IutD6wqQ== X-Received: by 2002:a05:6870:200f:b0:1bf:9f6:b810 with SMTP id o15-20020a056870200f00b001bf09f6b810mr16535724oab.36.1698786344044; Tue, 31 Oct 2023 14:05:44 -0700 (PDT) Received: from freyr.lechnology.com (ip98-183-112-25.ok.ok.cox.net. [98.183.112.25]) by smtp.gmail.com with ESMTPSA id ky10-20020a056871c4ca00b001efce0658e6sm24616oac.39.2023.10.31.14.05.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 Oct 2023 14:05:43 -0700 (PDT) From: David Lechner <dlechner@baylibre.com> To: Jonathan Cameron <jic23@kernel.org> Cc: David Lechner <dlechner@baylibre.com>, Alexandru Ardelean <alexandru.ardelean@analog.com>, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] iio: triggered-buffer: prevent possible freeing of wrong buffer Date: Tue, 31 Oct 2023 16:05:19 -0500 Message-ID: <20231031210521.1661552-1-dlechner@baylibre.com> X-Mailer: git-send-email 2.42.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Tue, 31 Oct 2023 14:06:04 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781306618119230625 X-GMAIL-MSGID: 1781306618119230625 |
Series |
iio: triggered-buffer: prevent possible freeing of wrong buffer
|
|
Commit Message
David Lechner
Oct. 31, 2023, 9:05 p.m. UTC
Commit ee708e6baacd ("iio: buffer: introduce support for attaching more
IIO buffers") introduced support for multiple buffers per indio_dev but
left indio_dev->buffer for a few legacy use cases.
In the case of the triggered buffer, iio_triggered_buffer_cleanup()
still assumes that indio_dev->buffer points to the buffer allocated by
iio_triggered_buffer_setup_ext(). However, since
iio_triggered_buffer_setup_ext() now calls iio_device_attach_buffer()
to attach the buffer, indio_dev->buffer will only point to the buffer
allocated by iio_device_attach_buffer() if it the first buffer attached.
This adds a check to make sure that no other buffer has been attached
yet to ensure that indio_dev->buffer will be assigned when
iio_device_attach_buffer() is called.
Fixes: ee708e6baacd ("iio: buffer: introduce support for attaching more IIO buffers")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
drivers/iio/buffer/industrialio-triggered-buffer.c | 10 ++++++++++
1 file changed, 10 insertions(+)
Comments
On Tue, 2023-10-31 at 16:05 -0500, David Lechner wrote: > Commit ee708e6baacd ("iio: buffer: introduce support for attaching more > IIO buffers") introduced support for multiple buffers per indio_dev but > left indio_dev->buffer for a few legacy use cases. > > In the case of the triggered buffer, iio_triggered_buffer_cleanup() > still assumes that indio_dev->buffer points to the buffer allocated by > iio_triggered_buffer_setup_ext(). However, since > iio_triggered_buffer_setup_ext() now calls iio_device_attach_buffer() > to attach the buffer, indio_dev->buffer will only point to the buffer > allocated by iio_device_attach_buffer() if it the first buffer attached. > > This adds a check to make sure that no other buffer has been attached > yet to ensure that indio_dev->buffer will be assigned when > iio_device_attach_buffer() is called. > > Fixes: ee708e6baacd ("iio: buffer: introduce support for attaching more IIO > buffers") > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > drivers/iio/buffer/industrialio-triggered-buffer.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c > b/drivers/iio/buffer/industrialio-triggered-buffer.c > index c7671b1f5ead..c06515987e7a 100644 > --- a/drivers/iio/buffer/industrialio-triggered-buffer.c > +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c > @@ -46,6 +46,16 @@ int iio_triggered_buffer_setup_ext(struct iio_dev > *indio_dev, > struct iio_buffer *buffer; > int ret; > > + /* > + * iio_triggered_buffer_cleanup() assumes that the buffer allocated > here > + * is assigned to indio_dev->buffer but this is only the case if this > + * function is the first caller to iio_device_attach_buffer(). If > + * indio_dev->buffer is already set then we can't proceed otherwise > the > + * cleanup function will try to free a buffer that was not allocated > here. > + */ > + if (indio_dev->buffer) > + return -EADDRINUSE; > + Hmmm, good catch! But I think this is just workarounding the real problem because like this, you can only have a triggered buffer by device. This should be fine as we don't really have any multi buffer user so far but ideally it should be possible. Long term we might want to think about moving 'pollfunc' to be a per buffer thing. Not sure how much trouble that would be given that a trigger is also per device and I don't know if it would make sense to have a trigger per buffer?! Ideally, given the multi buffer concept, I would say it makes sense but it might be difficult to accomplish. So better to think about it only if there's a real usecase for it. On thing that I guess it could be done is to change the triggered API so it returns a buffer and so iio_triggered_buffer_cleanup() would also get a pointer to the buffer it allocated (similar to what DMA buffer's are doing). But that's indeed also bigger change... Bahh, I'm likely over complicating things for now. Fell free to: Acked-by: Nuno Sa <nuno.sa@analog.com>
On Thu, Nov 2, 2023 at 3:59 AM Nuno Sá <noname.nuno@gmail.com> wrote: > > On Tue, 2023-10-31 at 16:05 -0500, David Lechner wrote: > > Commit ee708e6baacd ("iio: buffer: introduce support for attaching more > > IIO buffers") introduced support for multiple buffers per indio_dev but > > left indio_dev->buffer for a few legacy use cases. > > > > In the case of the triggered buffer, iio_triggered_buffer_cleanup() > > still assumes that indio_dev->buffer points to the buffer allocated by > > iio_triggered_buffer_setup_ext(). However, since > > iio_triggered_buffer_setup_ext() now calls iio_device_attach_buffer() > > to attach the buffer, indio_dev->buffer will only point to the buffer > > allocated by iio_device_attach_buffer() if it the first buffer attached. > > > > This adds a check to make sure that no other buffer has been attached > > yet to ensure that indio_dev->buffer will be assigned when > > iio_device_attach_buffer() is called. > > > > Fixes: ee708e6baacd ("iio: buffer: introduce support for attaching more IIO > > buffers") > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > --- > > drivers/iio/buffer/industrialio-triggered-buffer.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c > > b/drivers/iio/buffer/industrialio-triggered-buffer.c > > index c7671b1f5ead..c06515987e7a 100644 > > --- a/drivers/iio/buffer/industrialio-triggered-buffer.c > > +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c > > @@ -46,6 +46,16 @@ int iio_triggered_buffer_setup_ext(struct iio_dev > > *indio_dev, > > struct iio_buffer *buffer; > > int ret; > > > > + /* > > + * iio_triggered_buffer_cleanup() assumes that the buffer allocated > > here > > + * is assigned to indio_dev->buffer but this is only the case if this > > + * function is the first caller to iio_device_attach_buffer(). If > > + * indio_dev->buffer is already set then we can't proceed otherwise > > the > > + * cleanup function will try to free a buffer that was not allocated > > here. > > + */ > > + if (indio_dev->buffer) > > + return -EADDRINUSE; > > + > > Hmmm, good catch! But I think this is just workarounding the real problem Yes, I could have done a better job explaining my reason for this fix. It seemed like the simplest fix that could be easily backported to stable kernels. And then we can look at removing the legacy field completely in the future. > because like this, you can only have a triggered buffer by device. This should > be fine as we don't really have any multi buffer user so far but ideally it > should be possible. > > Long term we might want to think about moving 'pollfunc' to be a per buffer > thing. Not sure how much trouble that would be given that a trigger is also per > device and I don't know if it would make sense to have a trigger per buffer?! > Ideally, given the multi buffer concept, I would say it makes sense but it might > be difficult to accomplish. So better to think about it only if there's a real > usecase for it. > > On thing that I guess it could be done is to change the triggered API so it > returns a buffer and so iio_triggered_buffer_cleanup() would also get a pointer > to the buffer it allocated (similar to what DMA buffer's are doing). But that's > indeed also bigger change... Bahh, I'm likely over complicating things for now. This sounds very much like the work I am doing on SPI Engine offload support - having a trigger associated with a buffer. So maybe something will come out of that. ¯\_(ツ)_/¯ > Fell free to: > > Acked-by: Nuno Sa <nuno.sa@analog.com> > >
On Thu, 2 Nov 2023 09:53:13 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Thu, Nov 2, 2023 at 3:59 AM Nuno Sá <noname.nuno@gmail.com> wrote: > > > > On Tue, 2023-10-31 at 16:05 -0500, David Lechner wrote: > > > Commit ee708e6baacd ("iio: buffer: introduce support for attaching more > > > IIO buffers") introduced support for multiple buffers per indio_dev but > > > left indio_dev->buffer for a few legacy use cases. > > > > > > In the case of the triggered buffer, iio_triggered_buffer_cleanup() > > > still assumes that indio_dev->buffer points to the buffer allocated by > > > iio_triggered_buffer_setup_ext(). However, since > > > iio_triggered_buffer_setup_ext() now calls iio_device_attach_buffer() > > > to attach the buffer, indio_dev->buffer will only point to the buffer > > > allocated by iio_device_attach_buffer() if it the first buffer attached. > > > > > > This adds a check to make sure that no other buffer has been attached > > > yet to ensure that indio_dev->buffer will be assigned when > > > iio_device_attach_buffer() is called. > > > > > > Fixes: ee708e6baacd ("iio: buffer: introduce support for attaching more IIO > > > buffers") > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > --- > > > drivers/iio/buffer/industrialio-triggered-buffer.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c > > > b/drivers/iio/buffer/industrialio-triggered-buffer.c > > > index c7671b1f5ead..c06515987e7a 100644 > > > --- a/drivers/iio/buffer/industrialio-triggered-buffer.c > > > +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c > > > @@ -46,6 +46,16 @@ int iio_triggered_buffer_setup_ext(struct iio_dev > > > *indio_dev, > > > struct iio_buffer *buffer; > > > int ret; > > > > > > + /* > > > + * iio_triggered_buffer_cleanup() assumes that the buffer allocated > > > here > > > + * is assigned to indio_dev->buffer but this is only the case if this > > > + * function is the first caller to iio_device_attach_buffer(). If > > > + * indio_dev->buffer is already set then we can't proceed otherwise > > > the > > > + * cleanup function will try to free a buffer that was not allocated > > > here. > > > + */ > > > + if (indio_dev->buffer) > > > + return -EADDRINUSE; > > > + > > > > Hmmm, good catch! But I think this is just workarounding the real problem > > Yes, I could have done a better job explaining my reason for this fix. > It seemed like the simplest fix that could be easily backported to > stable kernels. And then we can look at removing the legacy field > completely in the future. > > > because like this, you can only have a triggered buffer by device. This should > > be fine as we don't really have any multi buffer user so far but ideally it > > should be possible. So far multibufferred devices have always been for cases where triggers don't make sense (devices with multiple hardware fifos that run out of step or where a single fifo is filled unevenly from different sensors, or big complex dma based devices where the trigger concept doesn't map at all.) I agree that it sort of make sense to make the trigger per buffer, but in practice I'm not sure on what sort of device will need it. Mind you, I guess you hit this in practice which rather implies something does! > > > > Long term we might want to think about moving 'pollfunc' to be a per buffer > > thing. Not sure how much trouble that would be given that a trigger is also per > > device and I don't know if it would make sense to have a trigger per buffer?! > > Ideally, given the multi buffer concept, I would say it makes sense but it might > > be difficult to accomplish. So better to think about it only if there's a real > > usecase for it. > > > > On thing that I guess it could be done is to change the triggered API so it > > returns a buffer and so iio_triggered_buffer_cleanup() would also get a pointer > > to the buffer it allocated (similar to what DMA buffer's are doing). But that's > > indeed also bigger change... Bahh, I'm likely over complicating things for now. > > This sounds very much like the work I am doing on SPI Engine offload > support - having a trigger associated with a buffer. So maybe > something will come out of that. ¯\_(ツ)_/¯ Ah. I guess if the trigger is being used to route things out of sight of software that might be a case where we do need multiple triggers per device... Doesn't sound 'too' hard to make work and we'll end up with similar case to buffers where iio_deviceX/current_trigger maps to the one for buffer0 so no ABI breakage, just new toys to play with. > > > Fell free to: > > > > Acked-by: Nuno Sa <nuno.sa@analog.com> > > > > Applied with a note that we may revisit adding multiple triggers support in future but that is unlikely to be suitable for a backport as it's a new feature. Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan
diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c index c7671b1f5ead..c06515987e7a 100644 --- a/drivers/iio/buffer/industrialio-triggered-buffer.c +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c @@ -46,6 +46,16 @@ int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev, struct iio_buffer *buffer; int ret; + /* + * iio_triggered_buffer_cleanup() assumes that the buffer allocated here + * is assigned to indio_dev->buffer but this is only the case if this + * function is the first caller to iio_device_attach_buffer(). If + * indio_dev->buffer is already set then we can't proceed otherwise the + * cleanup function will try to free a buffer that was not allocated here. + */ + if (indio_dev->buffer) + return -EADDRINUSE; + buffer = iio_kfifo_allocate(); if (!buffer) { ret = -ENOMEM;