Message ID | 20240110173958.4544-1-n.zhandarovich@fintech.ru (mailing list archive) |
---|---|
State | Changes Requested |
Headers |
Received: from sy.mirrors.kernel.org ([147.75.48.161]) by www.linuxtv.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from <linux-media+bounces-3479-patchwork=linuxtv.org@vger.kernel.org>) id 1rNcaf-000yZm-Ep for patchwork@linuxtv.org; Wed, 10 Jan 2024 17:42:10 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id 29C19B23E5F for <patchwork@linuxtv.org>; Wed, 10 Jan 2024 17:42:04 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 5763A4CDF8; Wed, 10 Jan 2024 17:41:49 +0000 (UTC) X-Original-To: linux-media@vger.kernel.org Received: from exchange.fintech.ru (exchange.fintech.ru [195.54.195.159]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 56FF24D102; Wed, 10 Jan 2024 17:41:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=fintech.ru Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=fintech.ru Received: from Ex16-01.fintech.ru (10.0.10.18) by exchange.fintech.ru (195.54.195.159) with Microsoft SMTP Server (TLS) id 14.3.498.0; Wed, 10 Jan 2024 20:40:06 +0300 Received: from localhost (10.0.253.138) by Ex16-01.fintech.ru (10.0.10.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4; Wed, 10 Jan 2024 20:40:06 +0300 From: Nikita Zhandarovich <n.zhandarovich@fintech.ru> To: Mauro Carvalho Chehab <mchehab@kernel.org> CC: Nikita Zhandarovich <n.zhandarovich@fintech.ru>, <linux-media@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH] media: em28xx: return error on media_device_register() failure Date: Wed, 10 Jan 2024 09:39:58 -0800 Message-ID: <20240110173958.4544-1-n.zhandarovich@fintech.ru> X-Mailer: git-send-email 2.25.1 Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-ClientProxiedBy: Ex16-02.fintech.ru (10.0.10.19) To Ex16-01.fintech.ru (10.0.10.18) X-LSpam-Score: -4.7 (----) X-LSpam-Report: No, score=-4.7 required=5.0 tests=BAYES_00=-1.9,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3 autolearn=unavailable autolearn_force=no |
Series |
media: em28xx: return error on media_device_register() failure
|
|
Commit Message
Nikita Zhandarovich
Jan. 10, 2024, 5:39 p.m. UTC
In an unlikely case of failure in media_device_register(), release
resources and return the erroneous value. Otherwise, possible issues
with registering the device will continue to be ignored.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support")
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
drivers/media/usb/em28xx/em28xx-cards.c | 2 ++
1 file changed, 2 insertions(+)
Comments
Le 10/01/2024 à 18:39, Nikita Zhandarovich a écrit : > In an unlikely case of failure in media_device_register(), release > resources and return the erroneous value. Otherwise, possible issues > with registering the device will continue to be ignored. > > Found by Linux Verification Center (linuxtesting.org) with static > analysis tool SVACE. > > Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > --- > drivers/media/usb/em28xx/em28xx-cards.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > index 4d037c92af7c..dae731dfc569 100644 > --- a/drivers/media/usb/em28xx/em28xx-cards.c > +++ b/drivers/media/usb/em28xx/em28xx-cards.c > @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf, > */ > #ifdef CONFIG_MEDIA_CONTROLLER > retval = media_device_register(dev->media_dev); > + if (retval) > + goto err_free; > #endif > > return 0; > > Hi, I think that some resources allocated in em28xx_init_dev() should also be freed if media_device_register() fails. (see the error handling path at the end of em28xx_init_dev()) Just my 2c. CJ
Em Wed, 10 Jan 2024 09:39:58 -0800 Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > In an unlikely case of failure in media_device_register(), release > resources and return the erroneous value. Otherwise, possible issues > with registering the device will continue to be ignored. > > Found by Linux Verification Center (linuxtesting.org) with static > analysis tool SVACE. > > Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > --- > drivers/media/usb/em28xx/em28xx-cards.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > index 4d037c92af7c..dae731dfc569 100644 > --- a/drivers/media/usb/em28xx/em28xx-cards.c > +++ b/drivers/media/usb/em28xx/em28xx-cards.c > @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf, > */ > #ifdef CONFIG_MEDIA_CONTROLLER > retval = media_device_register(dev->media_dev); > + if (retval) > + goto err_free; Not freeing resources here is intentional. See, the media controller API is optional on this driver. It will just provide a way to identify the device's topology, but the device is completely usable without it. Perhaps we need, instead, a patch documenting it, and preventing static analysis tools to point it as an issue. Thanks, Mauro
On 1/10/24 22:49, Mauro Carvalho Chehab wrote: > Em Wed, 10 Jan 2024 09:39:58 -0800 > Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > >> In an unlikely case of failure in media_device_register(), release >> resources and return the erroneous value. Otherwise, possible issues >> with registering the device will continue to be ignored. >> >> Found by Linux Verification Center (linuxtesting.org) with static >> analysis tool SVACE. >> >> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> >> --- >> drivers/media/usb/em28xx/em28xx-cards.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c >> index 4d037c92af7c..dae731dfc569 100644 >> --- a/drivers/media/usb/em28xx/em28xx-cards.c >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c >> @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf, >> */ >> #ifdef CONFIG_MEDIA_CONTROLLER >> retval = media_device_register(dev->media_dev); >> + if (retval) >> + goto err_free; > > Not freeing resources here is intentional. See, the media controller > API is optional on this driver. It will just provide a way to identify > the device's topology, but the device is completely usable without > it. > > Perhaps we need, instead, a patch documenting it, and preventing > static analysis tools to point it as an issue. > > Thanks, > Mauro Thank you for your feedback, however I had a few questions... While I understand what you mean about optional nature of media controller registration in this case, a quick glance into other calls to media_device_register() across the source code shows that usually failure with registering is handled as a proper error regardless of whether the device is still usable. But if you think that we can make an exception here, I'll happily oblige. Then if I am to continue on this path, would the following comment above the call to media_device_register() suffice? #ifdef CONFIG_MEDIA_CONTROLLER + /* + * No need to check the return value, the device will still be + * usable without media controller API. + */ retval = media_device_register(dev->media_dev); Thanks, Nikita
Em Thu, 11 Jan 2024 07:10:10 -0800 Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > On 1/10/24 22:49, Mauro Carvalho Chehab wrote: > > Em Wed, 10 Jan 2024 09:39:58 -0800 > > Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > > > >> In an unlikely case of failure in media_device_register(), release > >> resources and return the erroneous value. Otherwise, possible issues > >> with registering the device will continue to be ignored. > >> > >> Found by Linux Verification Center (linuxtesting.org) with static > >> analysis tool SVACE. > >> > >> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") > >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > >> --- > >> drivers/media/usb/em28xx/em28xx-cards.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > >> index 4d037c92af7c..dae731dfc569 100644 > >> --- a/drivers/media/usb/em28xx/em28xx-cards.c > >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c > >> @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf, > >> */ > >> #ifdef CONFIG_MEDIA_CONTROLLER > >> retval = media_device_register(dev->media_dev); > >> + if (retval) > >> + goto err_free; > > > > Not freeing resources here is intentional. See, the media controller > > API is optional on this driver. It will just provide a way to identify > > the device's topology, but the device is completely usable without > > it. > > > > Perhaps we need, instead, a patch documenting it, and preventing > > static analysis tools to point it as an issue. > > > > Thanks, > > Mauro > > Thank you for your feedback, however I had a few questions... > > While I understand what you mean about optional nature of media > controller registration in this case, a quick glance into other calls to > media_device_register() across the source code shows that usually > failure with registering is handled as a proper error regardless of > whether the device is still usable. But if you think that we can make an > exception here, I'll happily oblige. It depends on how the actual device is controlled. "Normal" media devices are fully controlled via v4l2 API. On those, the media controller API is there just to let userspace to query about the internal settings, but the actual pipelines are created via V4L2 API. Almost all normal applications will just ignore the media controller API. Embedded hardware, however, require setting pipelines via media controller for they to actually work. Almost all drivers implementing the media controller API fall on this category. > > Then if I am to continue on this path, would the following comment above > the call to media_device_register() suffice? > > #ifdef CONFIG_MEDIA_CONTROLLER > + /* > + * No need to check the return value, the device will still be + > * usable without media controller API. > + */ > retval = media_device_register(dev->media_dev); That works for me. It would still produce alerts at static analyzers, as they'll notice that we're storing retval there without actually using it. Thanks, Mauro
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c index 4d037c92af7c..dae731dfc569 100644 --- a/drivers/media/usb/em28xx/em28xx-cards.c +++ b/drivers/media/usb/em28xx/em28xx-cards.c @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf, */ #ifdef CONFIG_MEDIA_CONTROLLER retval = media_device_register(dev->media_dev); + if (retval) + goto err_free; #endif return 0;