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;
Date:   Wed, 31 Aug 2022 08:44:07 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Chuck Lever III <chuck.lever@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
Message-ID: <20220831054407.GI2071@kadam>
References: <122a5729fdcd76e23641c7d1853de2a632f6a742.1661509473.git.christophe.jaillet@wanadoo.fr>
 <20220826110808.GE2071@kadam>
 <5AAB19B0-0DAB-4313-AC9A-307E79CE4527@oracle.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5AAB19B0-0DAB-4313-AC9A-307E79CE4527@oracle.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?JpKJpIsnI8qIICpB43Xs7pYYhInoJKaOe1PRD2VGpLCkRFefx7LaEKUveq+Q?=
 =?us-ascii?Q?COhQ0qNkzcOB2zIyjlMR5L3EZh7rA1vxQp3uBmlFtK8BPK838SR9WdJhVwbH?=
 =?us-ascii?Q?GrgcTN5QqRSO18V+vHyyWHqYILn5q941bW+j6MzdFdOcSmSAIP33100+2kMM?=
 =?us-ascii?Q?R4mfy+ljqi/0oBxXox75rOXHFaQQVsA7nVtXN1J5QvJdsPwEQpU+Lyzyc9F5?=
 =?us-ascii?Q?uPIcS9p8MSYeVLlH7KDOZw6HHsVE1yA9cf8bdYpM6QxcyrY6Wf6TjDBCByQu?=
 =?us-ascii?Q?mar/FjE9r8HRQ1TQcdAsGFmeTyEfDWXzC1zj3mPflxzhZnwRDB5z49SdDYUx?=
 =?us-ascii?Q?sNPnqQ+BfywtKWv0ChrHMfvgeRMSbYOxtEEK5VndQexP1rkb00iEJ39GpvGA?=
 =?us-ascii?Q?5JAAXIuCpo8YlkE3rglwmnsByZ1a5mj3C8+K0aI60fjnSL5xqtdde8F2aNoN?=
 =?us-ascii?Q?uaF+Clp+hM8iSKMhoxWC7ELhHD4BWuct1RL58WL7MVsNbqIEclYCK04raCsC?=
 =?us-ascii?Q?8VRyMyPbDyy/3iNf99+ljJvwW6zICD300mRVEJ2wv5aKhJUHD8TmGg/AX0gr?=
 =?us-ascii?Q?L6TwjFkHSKg+3LX9k91ercR8QoGKn1mcuqK9DO/XV1qOEP0yDbTSL0xU8RYW?=
 =?us-ascii?Q?/wzmWxsFwkcQlMTT7vr+WJLex5fxBViq0st5EVlJdE8PIaHHzcmwYKt45tCu?=
 =?us-ascii?Q?zOc9Nw+L/3kuAxHlTqtsgfM7vuqo/F4ikbKPox1cSgbUupqpUwjV1mka3vgT?=
 =?us-ascii?Q?J8Z1AbspvHcnfsFu4fd3SeF/Ufhgo/ZavmIvO8z+M0e4z933hkKypq8RjsUY?=
 =?us-ascii?Q?VHGrNhKxZ96nahRTKitKqRQ6cBe+nhTYK/98l9ceoyR/R1vqflTGdioIrjEV?=
 =?us-ascii?Q?WIR5tPSJhjRqv4jXDg9xrqxbkz7bKAYPrDtJPKxsUdJOBJnc+mmS/dYD56+Y?=
 =?us-ascii?Q?VX+5s3/1hJOh7ni7jnnYiAb7P7n3ojdeus4Nzn3kfp+77bmuMPoIpMVYAQDI?=
 =?us-ascii?Q?fPuDUjdJ7Rtd829jUPtcyMKmnmu4CWQ9lt21Wwg1JCgNpO+D59QfDrpRDhyj?=
 =?us-ascii?Q?55G+L+c/DxBsJZC5CT+7j0viMMtUpWb2oDpc4IiFvhER7lLbCEAutU5ZuhcE?=
 =?us-ascii?Q?0i8dinYm90oYSB0LRw0D6pXPxRpGkTzw3IjYtWFta2X7cbjQJs37RXGNlRtE?=
 =?us-ascii?Q?lLQOVnQqiH82jrqXgiZ9VGxesuooSu0jlYTCmV64L+HDFYDqSXaE/Jo64Oa3?=
 =?us-ascii?Q?+7AClNv8UhIFI3CrplBiPDSOqmMkZEh4eowNKMw3XeFU8i3VAEbRCSUdib3J?=
 =?us-ascii?Q?KMlu9eAIho7gbcPomQW/HGmiQcczj4h3+yn+cHv2oF0pHzmVP1SVeRRoF65D?=
 =?us-ascii?Q?KHlBgY5l5hdaJQwVWAEbhXQuhO9+jdb4RSQ62qEM0EUYvBp75Vt7QPV/52ZS?=
 =?us-ascii?Q?BORq8qTHuK0eXQ1iJZLGB9QIcac7Ckqzkt6krQ0usBh5psOQjJZYscbw7eSZ?=
 =?us-ascii?Q?xSDkZWJBMjUrbrcO6eI1QWgG5LzCt0/MYlPuml8dy8QeJbLUdrWNVXQYU3Kk?=
 =?us-ascii?Q?Pqn88QpHF9KJUefP1uOlYHNmHPFuX/JwU4TuvyzUpzGxfGPgeCfFR7Geb6JD?=
 =?us-ascii?Q?lw=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 68a19242-5c7e-4228-c285-08da8b13d871
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Aug 2022 05:44:18.2382
 (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: qN1W1zYPEIHqqnmzuDOf+mIKjSON3y2XMj97rSSZisX+fOAevgNVBWW/++24BrE9np68CHozE929AqpNpmVaH4N3+weLMjimrXKpiTs4ys4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB6351
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-31_03,2022-08-30_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 phishscore=0
 mlxlogscore=999 adultscore=0 bulkscore=0 spamscore=0 malwarescore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2207270000 definitions=main-2208310027
X-Proofpoint-ORIG-GUID: i8ql36Mh-KCPH4I_fEUaaYz75BHfcDLK
X-Proofpoint-GUID: i8ql36Mh-KCPH4I_fEUaaYz75BHfcDLK
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 Tue, Aug 30, 2022 at 09:11:54PM +0000, Chuck Lever III wrote:
> 
> 
> > On Aug 26, 2022, at 7:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > 
> > 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 call
> >> a few lines above should be freed. Otherwise it leaks.
> >> 
> >> 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(-)
> >> 
> >> 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 = 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;
> > 
> > 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.

No, that's a different thing.  If anyone wants to clean that up then it
should be part of a different patch.

regards,
dan carpenter