Message ID | 5745d81c-3c06-4871-9785-12a469870934@web.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-12997-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:6f82:b0:100:9c79:88ff with SMTP id tb2csp2450842dyb; Fri, 29 Dec 2023 00:39:34 -0800 (PST) X-Google-Smtp-Source: AGHT+IHiEKco2I7TANc1WKikvxbkpLTr0H8tytnHbLDFiYq9tyboQn5hkd80Q9Ir/NwttOX2Rw43 X-Received: by 2002:a17:902:b784:b0:1d4:dbb:495d with SMTP id e4-20020a170902b78400b001d40dbb495dmr5719837pls.45.1703839174070; Fri, 29 Dec 2023 00:39:34 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1703839174; cv=none; d=google.com; s=arc-20160816; b=LJzgoL5LMNE9mgVHiwxCfiN3B/HWOx7CTnT6SiUggKmaZNn8aOqTRlShHFb9U4w+py uN1AqIuRSWwJ4kWMsYT4osCMfG/1RsRH92syEgOsgdyM4nvAhKYmGcaW2bzXFV77sASA 2f50wSJZ/sTLqMT2h9R9l62RLsX6xrD/TYtahcAPeqv7XaZuAz7IEO3miTEUYgHk15by lEKyy672wVFsqTvYRzVabV5HRlxYEVnZO4WRNQRQi6cehDeGgFwneyxM486hJGVwyREW tsycbwtWYiaXRITWx1OPszyKCHHavTpeGkPvlZprS4xqcoKEBK2O5XZXBZ07GtwBxDiQ Y6RQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=ui-outboundreport:content-transfer-encoding:in-reply-to:references :cc:to:from:content-language:subject:user-agent:mime-version :list-unsubscribe:list-subscribe:list-id:precedence:date:message-id :dkim-signature; bh=4eCIAUbjh9MTyXJKGiI+l552EPsAxzI6kAaXvQ8/w+4=; fh=z0OK8L6GyprJQYDPvIETb1Fe6dbfAA0C+AHWl1Zw7uA=; b=YY5fl/emDGxXBwaXqG9j1/TmCjI11c26mvTcW7IY6V7yk2mYw+hJj8BwabS2O4HAi5 KR3vdfAjiRnzTJRCru8o7sw8E0vW0eRzyjELf5SM/4jGswATEx3bSLM0GFLSvnNKHH+8 lf5b0L1eTVkBAmbVqI281p4vpriUmgDr1sYJrrPguQX+dMUFf6n/QuIaDmf3bcATJxBJ GalRSh3kqz6RCGxFOBwIS6xHt48aL6ieYsJr3YXS+xjQ/b4WEDpU+LGChWM7UCgkdOrM Y/2V+EPR29Ux4eL8nPnXGc6BTA9k2TfRd5GLojxfe972Ap+eTWgnSejNkOL9TKMCoVsb nZdQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@web.de header.s=s29768273 header.b=GTEGRq7E; spf=pass (google.com: domain of linux-kernel+bounces-12997-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-12997-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=web.de Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [2604:1380:40f1:3f00::1]) by mx.google.com with ESMTPS id 10-20020a170902c20a00b001d40773b09asi13478625pll.66.2023.12.29.00.39.33 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Dec 2023 00:39:34 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-12997-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@web.de header.s=s29768273 header.b=GTEGRq7E; spf=pass (google.com: domain of linux-kernel+bounces-12997-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-12997-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=web.de 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 6E005B22479 for <ouuuleilei@gmail.com>; Fri, 29 Dec 2023 08:39:18 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id AD58B8BF9; Fri, 29 Dec 2023 08:38:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=web.de header.i=markus.elfring@web.de header.b="GTEGRq7E" X-Original-To: linux-kernel@vger.kernel.org Received: from mout.web.de (mout.web.de [212.227.15.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F26517487; Fri, 29 Dec 2023 08:38:52 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=web.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=web.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=web.de; s=s29768273; t=1703839128; x=1704443928; i=markus.elfring@web.de; bh=W6yrPNc43adz34gAMxMy30lvqPV/Eba8/b5dIfUxezo=; h=X-UI-Sender-Class:Date:Subject:From:To:Cc:References: In-Reply-To; b=GTEGRq7E43WbnsOExZ3FmdrD+SQrDWgukQPygQzUBqPN0GgsmOK2QX/n/trtOu9J Tg1rESZnVGczPK8Y3R6s1iAWKE1Q7irHKp87HxjlOeyKNuemn0BWqiduqdiaw1Vum 3AKU3gFM9ruGWLNqPKNemD583a1YVvGjGF1gvRFyuCQlqa8IX6FTJ7kxYaUhEs5Mb WsQlloqImi8SckEc4QtVI6E+s19XbbpliOjlt4/dHDNsyHvUrjU1oWTpn8IeqeNQj 2vG7JFbPWMz89emLGcOtoOFymeohYdkdArN8685qj78fgCQkIXKVLOXOlfJG5QCwe to1v45rjUEzmKBSSzA== X-UI-Sender-Class: 814a7b36-bfc1-4dae-8640-3722d8ec6cd6 Received: from [192.168.178.21] ([94.31.85.95]) by smtp.web.de (mrweb006 [213.165.67.108]) with ESMTPSA (Nemesis) id 1MKdLM-1rXVTg0Ew8-00LPOS; Fri, 29 Dec 2023 09:38:48 +0100 Message-ID: <5745d81c-3c06-4871-9785-12a469870934@web.de> Date: Fri, 29 Dec 2023 09:38:47 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: [PATCH 2/2] virtiofs: Improve error handling in virtio_fs_get_tree() Content-Language: en-GB From: Markus Elfring <Markus.Elfring@web.de> To: virtualization@lists.linux.dev, linux-fsdevel@vger.kernel.org, kernel-janitors@vger.kernel.org, Miklos Szeredi <miklos@szeredi.hu>, Stefan Hajnoczi <stefanha@redhat.com>, Vivek Goyal <vgoyal@redhat.com> Cc: LKML <linux-kernel@vger.kernel.org> References: <c5c14b02-660a-46e1-9eb3-1a16d7c84922@web.de> In-Reply-To: <c5c14b02-660a-46e1-9eb3-1a16d7c84922@web.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Provags-ID: V03:K1:dIIBJp/wUeMxRDCbjzb05hZ0LyYzeW7/8EGr8swHME9Epop0uR0 XZKg5ysWpkbun+8jJjsN7ht/i7ypbe0HQ9ZbUd7jOZ+RUUT2kztJ9fsQoaYrs3dCBIj3YbO /5HbvkTEjiqjrOvLCRhz42fQgN8hkKr9jaHgkwh4lFtDk08OpaOUKx3+89gRGu3Wehu1nuT OlY9TOZhtJhF+e8yiCqhg== X-Spam-Flag: NO UI-OutboundReport: notjunk:1;M01:P0:rMTlxTdEg3Y=;mT6F3lBfUJnD7ni8owTQkOy+hlo 8kC2lDaDeta2038RkVjnn0wJr3oJRh1Kg4d/RGJ8dRkmEkM4xRWBOvd4DM7flB8RdCJbh7wLB vqk9DfK5d29E4k6c1RPShkYkRZxNsC01sH6j+ncv4U3A7OKe6zz4k3JCY270XCpJFYu7MTUgA YYnUhlIr/ZYQ1Zq4xYVkb3thHOmPGWJM0t8iL+ePMOXOPH05z1UKitcRxTn3TqhIQWkdOl3IR iZBvfVIKj1WjGvlO+42BkKLvF7rQtT8kW0wRJPiKk8HMmS9pQ1Rv+5Ljrc0layX0UwPiWzG4t TMFXxoA/YOBYHawpxsv0GlsHBQns1G+8GxiJtDqB52D0IXi1X+Ch2ul+64wCF7HTDWBVs4W69 BqeU7Q4icFO6bjJ8MoNa73DS/wv67GfD0GTwAQksme6kks0bGA/Sk8yczi7jBtKcZFhqdb8qW GVadJ3FXlkx9pDZ8wtbD5/FNtTbnBSdtjSTsgdkafNRbPA6F9FU993L0YpdKqi72DIpqAFNwk WPJCZXZqP1q1X1cSceIaVYTY8wGR5oa+XycGCfsf5R/mUXrtWvMkX6zkWqYIHhXHl2664Xfug 5y8CZ16DbfkMZorabZCc6Skxv6xLKdE2/tsdNV7o+G1y0QO7a8ZKchzSX/pV/BzhdAkF7zcqV mP/oISwJnQCDpCNLgVrgGe999E+JrDs+J2gEejxb047zNIGgEx9g+zz4OpyyT2+bTNAx0lJwn R1bWuS78HlpjZ8IndQoiylOxxrKMhVJwe+t9B24Vxks88BKLFfuT0iMza0v1ZimFOQWeP2Bj4 PjAr1kFT+XJl2dVXxWYwEbI7KfY5GU2e7n4aZiM4zuT/uBmIPnaos8xoV+E+moOnrXkCpj9gv QgrqabNuqIVYpO1Cnj9fSKPN50eHryQRhjhN5qk3xACU1t8hfJXWpf57GqiBMYZ0CUQOtYjr/ KQq6kQ== X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1786604865602642751 X-GMAIL-MSGID: 1786604865602642751 |
Series |
None
|
|
Commit Message
Markus Elfring
Dec. 29, 2023, 8:38 a.m. UTC
From: Markus Elfring <elfring@users.sourceforge.net> Date: Fri, 29 Dec 2023 09:15:07 +0100 The kfree() function was called in two cases by the virtio_fs_get_tree() function during error handling even if the passed variable contained a null pointer. This issue was detected by using the Coccinelle software. * Thus use another label. * Move an error code assignment into an if branch. * Delete an initialisation (for the variable “fc”) which became unnecessary with this refactoring. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- fs/fuse/virtio_fs.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) -- 2.43.0
Comments
On Fri, Dec 29, 2023 at 09:38:47AM +0100, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 29 Dec 2023 09:15:07 +0100 > > The kfree() function was called in two cases by > the virtio_fs_get_tree() function during error handling > even if the passed variable contained a null pointer. So what? kfree(NULL) is perfectly acceptable. Are you trying to optimise an error path?
>> The kfree() function was called in two cases by >> the virtio_fs_get_tree() function during error handling >> even if the passed variable contained a null pointer. > > So what? kfree(NULL) is perfectly acceptable. I suggest to reconsider the usefulness of such a special function call. > Are you trying to optimise an error path? I would appreciate if further improvements can be achieved. https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources Regards, Markus
On Fri, Dec 29, 2023 at 10:10:08AM +0100, Markus Elfring wrote: > >> The kfree() function was called in two cases by > >> the virtio_fs_get_tree() function during error handling > >> even if the passed variable contained a null pointer. > > > > So what? kfree(NULL) is perfectly acceptable. > > I suggest to reconsider the usefulness of such a special function call. Can you be more explicit in your suggestion?
>>> So what? kfree(NULL) is perfectly acceptable. >> >> I suggest to reconsider the usefulness of such a special function call. > > Can you be more explicit in your suggestion? I hope that the change acceptance can grow for the presented transformation. Are you looking for an improved patch description? Regards, Markus
On Tue, Jan 02, 2024 at 10:35:17AM +0100, Markus Elfring wrote: > >>> So what? kfree(NULL) is perfectly acceptable. > >> > >> I suggest to reconsider the usefulness of such a special function call. > > > > Can you be more explicit in your suggestion? > > I hope that the change acceptance can grow for the presented transformation. > Are you looking for an improved patch description? Do you consider more clarity in your argumentation?
> Do you consider more clarity in your argumentation?
It is probably clear that the function call “kfree(NULL)” does not perform
data processing which is really useful for the caller.
Such a call is kept in some cases because programmers did not like to invest
development resources for its avoidance.
Regards,
Markus
On Tue, Jan 02, 2024 at 11:47:38AM +0100, Markus Elfring wrote: > > Do you consider more clarity in your argumentation? > > It is probably clear that the function call “kfree(NULL)” does not perform > data processing which is really useful for the caller. > Such a call is kept in some cases because programmers did not like to invest > development resources for its avoidance. on the contrary, it is extremely useful for callers to not have to perform the NULL check themselves. It also mirrors userspace where free(NULL) is valid according to ISO/ANSI C, so eases the transition for programmers who are coming from userspace. It costs nothing in the implementation as it is part of the check for the ZERO_PTR. And from a practical point of view, we can't take it out now. We can never find all the places which assume the current behaviour. So since we must keep kfree(NULL) working, we should take advantage of that to simplify users.
>> It is probably clear that the function call “kfree(NULL)” does not perform >> data processing which is really useful for the caller. >> Such a call is kept in some cases because programmers did not like to invest >> development resources for its avoidance. > > on the contrary, it is extremely useful for callers to not have to perform > the NULL check themselves. Some function calls indicate a resource allocation failure by a null pointer. Such pointer checks are generally performed for error detection so that appropriate exception handling can be chosen. > It also mirrors userspace where free(NULL) > is valid according to ISO/ANSI C, so eases the transition for programmers > who are coming from userspace. It costs nothing in the implementation > as it is part of the check for the ZERO_PTR. How many development efforts do you dare to spend on more complete and efficient error/exception handling? Regards, Markus
On Fri, Dec 29, 2023 at 09:38:47AM +0100, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 29 Dec 2023 09:15:07 +0100 > > The kfree() function was called in two cases by > the virtio_fs_get_tree() function during error handling > even if the passed variable contained a null pointer. > This issue was detected by using the Coccinelle software. > > * Thus use another label. > > * Move an error code assignment into an if branch. > > * Delete an initialisation (for the variable “fc”) > which became unnecessary with this refactoring. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> As Matthew said that kfree(NULL) is perfectly acceptable usage in kernel, so I really don't feel that this patch is required. Current code looks good as it is. Thanks Vivek > --- > fs/fuse/virtio_fs.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > index 2f8ba9254c1e..0746f54ec743 100644 > --- a/fs/fuse/virtio_fs.c > +++ b/fs/fuse/virtio_fs.c > @@ -1415,10 +1415,10 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > { > struct virtio_fs *fs; > struct super_block *sb; > - struct fuse_conn *fc = NULL; > + struct fuse_conn *fc; > struct fuse_mount *fm; > unsigned int virtqueue_size; > - int err = -EIO; > + int err; > > /* This gets a reference on virtio_fs object. This ptr gets installed > * in fc->iq->priv. Once fuse_conn is going away, it calls ->put() > @@ -1431,13 +1431,15 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > } > > virtqueue_size = virtqueue_get_vring_size(fs->vqs[VQ_REQUEST].vq); > - if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD)) > - goto out_err; > + if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD)) { > + err = -EIO; > + goto lock_mutex; > + } > > err = -ENOMEM; > fc = kzalloc(sizeof(*fc), GFP_KERNEL); > if (!fc) > - goto out_err; > + goto lock_mutex; > > fm = kzalloc(sizeof(*fm), GFP_KERNEL); > if (!fm) > @@ -1476,6 +1478,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > > out_err: > kfree(fc); > +lock_mutex: > mutex_lock(&virtio_fs_mutex); > virtio_fs_put(fs); > mutex_unlock(&virtio_fs_mutex); > -- > 2.43.0 >
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 2f8ba9254c1e..0746f54ec743 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -1415,10 +1415,10 @@ static int virtio_fs_get_tree(struct fs_context *fsc) { struct virtio_fs *fs; struct super_block *sb; - struct fuse_conn *fc = NULL; + struct fuse_conn *fc; struct fuse_mount *fm; unsigned int virtqueue_size; - int err = -EIO; + int err; /* This gets a reference on virtio_fs object. This ptr gets installed * in fc->iq->priv. Once fuse_conn is going away, it calls ->put() @@ -1431,13 +1431,15 @@ static int virtio_fs_get_tree(struct fs_context *fsc) } virtqueue_size = virtqueue_get_vring_size(fs->vqs[VQ_REQUEST].vq); - if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD)) - goto out_err; + if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD)) { + err = -EIO; + goto lock_mutex; + } err = -ENOMEM; fc = kzalloc(sizeof(*fc), GFP_KERNEL); if (!fc) - goto out_err; + goto lock_mutex; fm = kzalloc(sizeof(*fm), GFP_KERNEL); if (!fm) @@ -1476,6 +1478,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) out_err: kfree(fc); +lock_mutex: mutex_lock(&virtio_fs_mutex); virtio_fs_put(fs); mutex_unlock(&virtio_fs_mutex);