| Message ID | 1521176303-17546-1-git-send-email-ji_hun.kim@samsung.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Laurent Pinchart |
| Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1ewhSD-0002nS-0d; Fri, 16 Mar 2018 04:58:57 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751310AbeCPE6r (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 16 Mar 2018 00:58:47 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:42008 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750839AbeCPE6o (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 16 Mar 2018 00:58:44 -0400 Received: from epcas1p4.samsung.com (unknown [182.195.41.48]) by mailout3.samsung.com (KnoxPortal) with ESMTP id 20180316045842epoutp0392725ea4f58df57c4a446597b9c3afec~cTc4y5VID2363523635epoutp03L; Fri, 16 Mar 2018 04:58:42 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout3.samsung.com 20180316045842epoutp0392725ea4f58df57c4a446597b9c3afec~cTc4y5VID2363523635epoutp03L DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1521176322; bh=CmdwloBy1xty/Jdltdtd39oRR0FjPBWV7W9VfobTrlc=; h=From:To:Cc:Subject:Date:References:From; b=PT2noQ6YEvUBnGM6tNzGaTLSECCureWtFGGTEd8YVqbGQIwkfJAitgOW0z1CTiMQu nTCmF+cXTnv1hYaE/7GIsYt5CY6tvcPILlwvnppoeDfRaRns2PPtDfkosF6IRze5sR jkbWTznKLOaIhuwFT0zynjsyW0rHWoxr+1xkqBeE= Received: from epsmges2p4.samsung.com (unknown [182.195.40.60]) by epcas1p1.samsung.com (KnoxPortal) with ESMTP id 20180316045841epcas1p1c29041309912d1eaeb0abc99b56d6424~cTc4UsG7v1096010960epcas1p1r; Fri, 16 Mar 2018 04:58:41 +0000 (GMT) Received: from epcas2p3.samsung.com ( [182.195.41.55]) by epsmges2p4.samsung.com (Symantec Messaging Gateway) with SMTP id D6.BE.04080.10F4BAA5; Fri, 16 Mar 2018 13:58:41 +0900 (KST) Received: from epsmgms2p2new.samsung.com (unknown [182.195.42.143]) by epcas2p3.samsung.com (KnoxPortal) with ESMTP id 20180316045841epcas2p34dc11231c65e2032e88ac7138db2daee~cTc31UnTA0236702367epcas2p3R; Fri, 16 Mar 2018 04:58:41 +0000 (GMT) X-AuditID: b6c32a48-9adff70000000ff0-9e-5aab4f012149 Received: from epmmp1.local.host ( [203.254.227.16]) by epsmgms2p2new.samsung.com (Symantec Messaging Gateway) with SMTP id 6C.D0.03890.00F4BAA5; Fri, 16 Mar 2018 13:58:41 +0900 (KST) Received: from localhost.localdomain ([10.253.107.61]) by mmp1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0P5O00BL435J8LB0@mmp1.samsung.com>; Fri, 16 Mar 2018 13:58:40 +0900 (KST) From: Ji-Hun Kim <ji_hun.kim@samsung.com> To: mchehab@kernel.org Cc: gregkh@linuxfoundation.org, arvind.yadav.cs@gmail.com, ji_hun.kim@samsung.com, linux-media@vger.kernel.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure Date: Fri, 16 Mar 2018 13:58:23 +0900 Message-id: <1521176303-17546-1-git-send-email-ji_hun.kim@samsung.com> X-Mailer: git-send-email 1.9.1 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrKKsWRmVeSWpSXmKPExsWy7bCmuS6j/+oog/63qhbXehcyW+w584vd onnxejaL/tPbGS223pK2uLxrDptFz4atrBbLNv1hcuDwuLfvMIvHzll32T02repk89g/dw27 R9+WVYwenzfJBbBFpdpkpCampBYppOYl56dk5qXbKnkHxzvHm5oZGOoaWlqYKynkJeam2iq5 +AToumXmAN2jpFCWmFMKFApILC5W0rezKcovLUlVyMgvLrFVijY0NNIzNDDXMzIy0jMxj7Uy MgUqSUjNmHDoOEvBWq6K1492sTQwbuboYuTkkBAwkZg2q5Wpi5GLQ0hgB6PEyX+/oJzvjBKn vv1mhql6dvYtG0RiN6PE0r3zGUESQgI/GCXePXYFsdkENCU2dl8Di4sIiEk82vaKBaSBWeA8 o0TPsUdgk4QFwiROHNjGDmKzCKhKHLz7lwXE5hVwk7jTd5wdYpucxMljk1lBmiUE/rJKdM3+ xQaRcJF49/0ElC0s8er4FqgGaYlnqzYyQtjVEguu7GCBsGskbv5fygRhG0v09lwAO4JZgE+i 4/BfoF4OoDivREebEESJh0THshusELajxKMbn1kgnoyVWDO5iWUCo+QCRoZVjGKpBcW56anF RgUmesWJucWleel6yfm5mxjBSUTLYwfjgXM+hxgFOBiVeHgzmldFCbEmlhVX5h5ilOBgVhLh 9X8GFOJNSaysSi3Kjy8qzUktPsRoCgyOicxSosn5wASXVxJvaGJpYGJmZmhuZGpgriTO2xbg EiUkkJ5YkpqdmlqQWgTTx8TBKdXAWCQ5N6TptFqt++1m6Wgdxe2cegrnZuyeXu6l8kf4acMG mTtvaxc+dvmanHHx9L2Eu1PuZXm+S8/fo39EN8Hr8J7ittOucaF1UZxZSbzFHQUxvNZypp8e zPnw+v358s3bKnYs4m/dsl+XNWDnu5fd2bveLVX5+e7QyV9y0UwHdqp0mwXEsgfvUWIpzkg0 1GIuKk4EAFWdAAg4AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrHJMWRmVeSWpSXmKPExsVy+t9jAV1G/9VRBns2sVpc613IbLHnzC92 i+bF69ks+k9vZ7TYekva4vKuOWwWPRu2slos2/SHyYHD496+wyweO2fdZffYtKqTzWP/3DXs Hn1bVjF6fN4kF8AWxWWTkpqTWZZapG+XwJUx4dBxloK1XBWvH+1iaWDczNHFyMkhIWAi8ezs W7YuRi4OIYGdjBIzup+zQDg/GCXa57WwglSxCWhKbOy+xghiiwiISTza9gqsiFngPKPE3dnH WUASwgJhEicObGMHsVkEVCUO3v0LFucVcJO403ecHWKdnMTJY5NZJzByLWBkWMUomVpQnJue W2xUYJSXWq5XnJhbXJqXrpecn7uJERgq2w5r9e9gfLwk/hCjAAejEg9vRvOqKCHWxLLiytxD jBIczEoivP7PgEK8KYmVValF+fFFpTmpxYcYpTlYlMR5+fOPRQoJpCeWpGanphakFsFkmTg4 pRoY90QwqCrZspYcfTjr+J73WyXD322Jfbd3sWd17zqbr7Z2i7/NeJB8si+kto6Hk1971brJ ud+sky6pHzW9G5NY17Y8wXGV28oy9aT6mh3lKmeL+W6XvXLbJLsyLuXiqimHwiWUXp1/wmEw wXraEZOZM3iTcy4b5NX9cnF5pd82TarDVMmFs6hAiaU4I9FQi7moOBEAwAoIRxECAAA= X-CMS-MailID: 20180316045841epcas2p34dc11231c65e2032e88ac7138db2daee X-Msg-Generator: CA CMS-TYPE: 102P DLP-Filter: Pass X-CFilter-Loop: Reflected X-CMS-RootMailID: 20180316045841epcas2p34dc11231c65e2032e88ac7138db2daee X-RootMTR: 20180316045841epcas2p34dc11231c65e2032e88ac7138db2daee References: <CGME20180316045841epcas2p34dc11231c65e2032e88ac7138db2daee@epcas2p3.samsung.com> Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Ji-Hun Kim
March 16, 2018, 4:58 a.m. UTC
There is no failure checking on the param value which will be allocated
memory by kmalloc. Add a null pointer checking statement. Then goto error:
and return -ENOMEM error code when kmalloc is failed.
Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com>
---
drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8 ++++++++
1 file changed, 8 insertions(+)
Comments
On Fri, Mar 16, 2018 at 01:58:23PM +0900, Ji-Hun Kim wrote: > There is no failure checking on the param value which will be allocated > memory by kmalloc. Add a null pointer checking statement. Then goto error: > and return -ENOMEM error code when kmalloc is failed. > > Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com> > --- > drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > index 6a3434c..55a922c 100644 > --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > @@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) > > params = kmalloc(sizeof(struct ipipe_module_params), > GFP_KERNEL); > + if (!params) { > + rval = -ENOMEM; > + goto error; ^^^^^^^^^^ What does "goto error" do, do you think? It's not clear from the name. When you have an unclear goto like this it often means the error handling is going to be buggy. In this case, it does nothing so a direct "return -ENOMEM;" would be more clear. But the rest of the error handling is buggy. 1263 static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) 1264 { 1265 struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd); 1266 unsigned int i; 1267 int rval = 0; 1268 1269 for (i = 0; i < ARRAY_SIZE(ipipe_modules); i++) { 1270 unsigned int bit = 1 << i; 1271 1272 if (cfg->flag & bit) { 1273 const struct ipipe_module_if *module_if = 1274 &ipipe_modules[i]; 1275 struct ipipe_module_params *params; 1276 void __user *from = *(void * __user *) 1277 ((void *)cfg + module_if->config_offset); 1278 size_t size; 1279 void *to; 1280 1281 params = kmalloc(sizeof(struct ipipe_module_params), 1282 GFP_KERNEL); Do a direct return: if (!params) return -ENOMEM; 1283 to = (void *)params + module_if->param_offset; 1284 size = module_if->param_size; 1285 1286 if (to && from && size) { 1287 if (copy_from_user(to, from, size)) { 1288 rval = -EFAULT; 1289 break; The most recent thing we allocated is "params" so lets do a "goto free_params;". We'll have to declare "params" at the start of the function instead inside this block. 1290 } 1291 rval = module_if->set(ipipe, to); 1292 if (rval) 1293 goto error; goto free_params again since params is still the most recent thing we allocated. 1294 } else if (to && !from && size) { 1295 rval = module_if->set(ipipe, NULL); 1296 if (rval) 1297 goto error; And here again goto free_params. 1298 } 1299 kfree(params); 1300 } 1301 } 1302 error: 1303 return rval; Change this to: return 0; free_params: kfree(params); return rval; 1304 } regards, dan carpenter
On Fri, Mar 16, 2018 at 11:32:34AM +0300, Dan Carpenter wrote: > On Fri, Mar 16, 2018 at 01:58:23PM +0900, Ji-Hun Kim wrote: > > There is no failure checking on the param value which will be allocated > > memory by kmalloc. Add a null pointer checking statement. Then goto error: > > and return -ENOMEM error code when kmalloc is failed. > > > > Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com> > > --- > > drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > > index 6a3434c..55a922c 100644 > > --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > > +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > > @@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) > > > > params = kmalloc(sizeof(struct ipipe_module_params), > > GFP_KERNEL); > > + if (!params) { > > + rval = -ENOMEM; > > + goto error; > ^^^^^^^^^^ > > What does "goto error" do, do you think? It's not clear from the name. > When you have an unclear goto like this it often means the error > handling is going to be buggy. > > In this case, it does nothing so a direct "return -ENOMEM;" would be > more clear. But the rest of the error handling is buggy. Hi Dan, I appreciate for your specific feedbacks. It looks more clear. And I'd like you to see my question below. I will send the patch v2. > > 1263 static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) > 1264 { > 1265 struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd); > 1266 unsigned int i; > 1267 int rval = 0; > 1268 > 1269 for (i = 0; i < ARRAY_SIZE(ipipe_modules); i++) { > 1270 unsigned int bit = 1 << i; > 1271 > 1272 if (cfg->flag & bit) { > 1273 const struct ipipe_module_if *module_if = > 1274 &ipipe_modules[i]; > 1275 struct ipipe_module_params *params; > 1276 void __user *from = *(void * __user *) > 1277 ((void *)cfg + module_if->config_offset); > 1278 size_t size; > 1279 void *to; > 1280 > 1281 params = kmalloc(sizeof(struct ipipe_module_params), > 1282 GFP_KERNEL); > > Do a direct return: > > if (!params) > return -ENOMEM; > > 1283 to = (void *)params + module_if->param_offset; > 1284 size = module_if->param_size; > 1285 > 1286 if (to && from && size) { > 1287 if (copy_from_user(to, from, size)) { > 1288 rval = -EFAULT; > 1289 break; > > The most recent thing we allocated is "params" so lets do a > "goto free_params;". We'll have to declare "params" at the start of the > function instead inside this block. > > 1290 } > 1291 rval = module_if->set(ipipe, to); > 1292 if (rval) > 1293 goto error; > > goto free_params again since params is still the most recent thing we > allocated. > > 1294 } else if (to && !from && size) { > 1295 rval = module_if->set(ipipe, NULL); > 1296 if (rval) > 1297 goto error; > > And here again goto free_params. > > 1298 } > 1299 kfree(params); > 1300 } > 1301 } > 1302 error: > 1303 return rval; > > > Change this to: > > return 0; Instead of returning rval, returning 0 would be fine? It looks that should return rval in normal case. > > free_params: > kfree(params); > return rval; > > 1304 } > > regards, > dan carpenter > > Thanks, Ji-Hun
On Mon, Mar 19, 2018 at 01:24:57PM +0900, Ji-Hun Kim wrote: > > 1294 } else if (to && !from && size) { > > 1295 rval = module_if->set(ipipe, NULL); > > 1296 if (rval) > > 1297 goto error; > > > > And here again goto free_params. > > > > 1298 } > > 1299 kfree(params); > > 1300 } > > 1301 } > > 1302 error: > > 1303 return rval; > > > > > > Change this to: > > > > return 0; > Instead of returning rval, returning 0 would be fine? It looks that should > return rval in normal case. > In the proposed code, the errors all do a return or a goto so "rval" would be zero here. Then the error path would look like: err_free_params: kfree(params); return rval; } regards, dan carpenter
diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c index 6a3434c..55a922c 100644 --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c @@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) params = kmalloc(sizeof(struct ipipe_module_params), GFP_KERNEL); + if (!params) { + rval = -ENOMEM; + goto error; + } to = (void *)params + module_if->param_offset; size = module_if->param_size; @@ -1323,6 +1327,10 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) params = kmalloc(sizeof(struct ipipe_module_params), GFP_KERNEL); + if (!params) { + rval = -ENOMEM; + goto error; + } from = (void *)params + module_if->param_offset; size = module_if->param_size;