Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
CC:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        Jeff Layton <jlayton@kernel.org>,
        Bruce Fields <bfields@redhat.com>,
        Scott Mayhew <smayhew@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] nfsd: Fix a memory leak in an error handling path
Thread-Topic: [PATCH] nfsd: Fix a memory leak in an error handling path
Thread-Index: AQHYuTYa4ohD5b5XPEGU215z45UTM63BBdIAgAbyBAA=
Date:   Tue, 30 Aug 2022 21:11:54 +0000
Message-ID: <5AAB19B0-0DAB-4313-AC9A-307E79CE4527@oracle.com>
References: <122a5729fdcd76e23641c7d1853de2a632f6a742.1661509473.git.christophe.jaillet@wanadoo.fr>
 <20220826110808.GE2071@kadam>
In-Reply-To: <20220826110808.GE2071@kadam>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?PRXdTDmNB0gZwWra81x92cTHQfafo36gwzOk4h+HF3GKiG4B71SJ99mtTS6g?=
 =?us-ascii?Q?9Ypt/qQhG8JdSn93UM8tj7Mag/QH3JSfOiRdbmX3dcqgwFqn0TeuK4NtfFxn?=
 =?us-ascii?Q?g/vuywuJ7nPRg70BsOyX7J5WIGtkIephGaxDbsWy6csmIjff431aO5KLyWUD?=
 =?us-ascii?Q?BqDPey1oEqeJr7u1LjrEzFML0QWXmPZMDl/qwUwJdIQIsudBN3DE1CMhRJ6j?=
 =?us-ascii?Q?4x0v2dC2PLRm4WBHsQD/jOMhhGtbeWO50sDy6MxPYVwLMAz/kQYbJGzOo+6s?=
 =?us-ascii?Q?SqWgypMbfE+saMfSy0gXqiVMnGnGHaDkAgjI3Rl0LXcP/dZj/HFeXdrKOw2N?=
 =?us-ascii?Q?NpoKqQSqo6KtvFtJ9rSSwwqag7keJ6X1Iv8BlWALu4SODnhBzidGx5n/690m?=
 =?us-ascii?Q?XgFxtg1gKR70udVOxcpQ32+vwu7EVuKHFHaDAjBPiSre2+e9AiknyLvKMTuV?=
 =?us-ascii?Q?lKTKjRowmy827aOE9ABqmZWY7tnMh9I2LxviPZ7tv1cjKLMWbKzz78XbApEB?=
 =?us-ascii?Q?IvqWaZkANaitdlB/UDJ+Kcg3mDp6TVcR6HH5E4AMRV93GjdQ+EHQTerUFA35?=
 =?us-ascii?Q?rMaPEK5Hw6Bt2Y8YGGA4aG9JNQsibnWWf4TEtxPwe1Ws2i2vlnwzJTzkMSvZ?=
 =?us-ascii?Q?YRt3evF2E7w9t3VKv0xdqlcoxyo0zG6ViNLd9TbVwjdXl8iovxTwsBPmzg+5?=
 =?us-ascii?Q?45vVWQBA03mQ4jhipc0JAhq/l/gDttZBbiCUrgvxOPtJWJQoMA9ZSHFQmyuZ?=
 =?us-ascii?Q?VGRW/Ga37Jl0CYeHYRAVUKHB2C4nPXkFWLJkyvc7lps6Pqm3THdOT3M89XqW?=
 =?us-ascii?Q?b3SDEogx+aBDeocLa6uJKkkj7sW3NDpke6LmAKcOSrWu3yLZ972Z9T5Aydw3?=
 =?us-ascii?Q?F1fyGt+f/K0jvwHSeE5Ig9SA3OeaHtGUA4QBdMr8BMmAZTPp0kQdQIvSB/z+?=
 =?us-ascii?Q?kLlcwO3BCSmOH9yTOVY5bX9hpH4yZ6qIbGbM2X7Zvz/g9NlLqnK39WgwN4nt?=
 =?us-ascii?Q?DtUzzokXI76EFvJofm1l+YLVS0aQHDGScVv4FA/UrCeuNtdCDf0gv6FG9wUU?=
 =?us-ascii?Q?pcHTewMDempI0PYf3wTYznygOxyfSSRLor84PGQ1zpR0fcjVIxygkbelPImR?=
 =?us-ascii?Q?kRrcyLo+YzsyF/z8j/2SgaaavqCTdSmfBBTFxwVQA7jVPv/qgVNUaAcWqAZA?=
 =?us-ascii?Q?ZAxBxn3t0oZ2yKeVJ97jmXT9a7WnaXTWQIOpLtIy/n/5eGY2zNnepezyHf/V?=
 =?us-ascii?Q?6BW2DUOZ8chMdLWfxxWzHNMLSMHoXU23EDKJHV/wZaG7ajXhKZ3DKiDQBamG?=
 =?us-ascii?Q?1hKRyU8XdrgX0McCbWdRfIXu3ZDI4N+uUarz6VXjOsqCbDJ8WWJbz9/sCxZ6?=
 =?us-ascii?Q?aM5k1U/JBpjKQ80FpTsTa2ga1FxOWAFKLzQJHFay/oFZpIM1wnM0HK197tLY?=
 =?us-ascii?Q?Eb/pE82OCisYMiezK3SPJ+dNXQ8xob1G3eUUJXRF9966Mr/D2nlmMNuF3kys?=
 =?us-ascii?Q?UzDHmqEZs9XnmlHY37pv9NpNd+aiB0hxL8H3/jT7NMhih4J3jP7In5Ok2Qj1?=
 =?us-ascii?Q?kQHyLJusB9Gq9mo/IGTE058g9EfyvgKKUbPzi0yKvfde7IW8IGzsxAOzBs67?=
 =?us-ascii?Q?jA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1CEB03E9E5194E4386087E7E181778FD@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: df293b7d-d70b-4f72-3f83-08da8acc4445
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Aug 2022 21:11:54.9384
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: qSsdklzybuwUkeHxvx5UFT0oxcfSaJLYABwr6dH4HL7/A7msCEjw0g8V6+i9ZpRb0JztWHf7MrxSFAvjUpwUBQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5065
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-30_12,2022-08-30_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxscore=0 malwarescore=0
 suspectscore=0 spamscore=0 adultscore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2207270000
 definitions=main-2208300095
X-Proofpoint-GUID: E6aDWT9VpFh64A8Rjl66vx7gDNoO8dML
X-Proofpoint-ORIG-GUID: E6aDWT9VpFh64A8Rjl66vx7gDNoO8dML
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Aug 26, 2022, at 7:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wro=
te:
>=20
> On Fri, Aug 26, 2022 at 12:24:54PM +0200, Christophe JAILLET wrote:
>> If this memdup_user() call fails, the memory allocated in a previous cal=
l
>> a few lines above should be freed. Otherwise it leaks.
>>=20
>> Fixes: 6ee95d1c8991 ("nfsd: add support for upcall version 2")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Speculative, untested.
>> ---
>> fs/nfsd/nfs4recover.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>=20
>> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
>> index b29d27eaa8a6..248ff9f4141c 100644
>> --- a/fs/nfsd/nfs4recover.c
>> +++ b/fs/nfsd/nfs4recover.c
>> @@ -815,8 +815,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg=
_v2 __user *cmsg,
>> 				princhash.data =3D memdup_user(
>> 						&ci->cc_princhash.cp_data,
>> 						princhashlen);
>> -				if (IS_ERR_OR_NULL(princhash.data))
>> +				if (IS_ERR_OR_NULL(princhash.data)) {
>> +					kfree(name.data);
>> 					return -EFAULT;
>=20
> This comment is not directed at you and is not related to your patch.
> But memdup_user() never returns NULL, only error pointers.  I wrote a
> fifteen page blog entry about NULL vs error pointers the other week.
> https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and=
-null/
> This should propagate the error code from memdup_user() instead of
> -EFAULT.

I take it then that Christophe should redrive this with your suggested
corrections? I haven't applied this yet because I was waiting for
follow-up.


--
Chuck Lever