From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 35810C5DF62 for ; Wed, 6 Nov 2019 10:38:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0CD5D20869 for ; Wed, 6 Nov 2019 10:38:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731303AbfKFKiz (ORCPT ); Wed, 6 Nov 2019 05:38:55 -0500 Received: from smtprelay0168.hostedemail.com ([216.40.44.168]:53329 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725937AbfKFKiy (ORCPT ); Wed, 6 Nov 2019 05:38:54 -0500 Received: from filter.hostedemail.com (clb03-v110.bra.tucows.net [216.40.38.60]) by smtprelay06.hostedemail.com (Postfix) with ESMTP id 5BDBF18224D68; Wed, 6 Nov 2019 10:38:53 +0000 (UTC) X-Session-Marker: 6A6F6540706572636865732E636F6D X-HE-Tag: fish39_6a8263ffbb247 X-Filterd-Recvd-Size: 3444 Received: from XPS-9350.home (unknown [47.151.135.224]) (Authenticated sender: joe@perches.com) by omf04.hostedemail.com (Postfix) with ESMTPA; Wed, 6 Nov 2019 10:38:51 +0000 (UTC) Message-ID: <6137855bb4170c438c7436cbdb7dfd21639a8855.camel@perches.com> Subject: Re: [PATCH] s390/pkey: Use memdup_user() rather than duplicating its implementation From: Joe Perches To: Markus Elfring , linux-s390@vger.kernel.org, Christian =?ISO-8859-1?Q?Borntr=E4ger?= , Harald Freudenberger , Heiko Carstens , Ingo Franzki , Vasily Gorbik Cc: LKML , kernel-janitors@vger.kernel.org Date: Wed, 06 Nov 2019 02:38:39 -0800 In-Reply-To: <08422b7e-2071-ee52-049e-c3ac55bc67a9@web.de> References: <08422b7e-2071-ee52-049e-c3ac55bc67a9@web.de> Content-Type: text/plain; charset="ISO-8859-1" User-Agent: Evolution 3.34.1-2 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2019-11-06 at 11:22 +0100, Markus Elfring wrote: > From: Markus Elfring > Date: Wed, 6 Nov 2019 11:11:42 +0100 > > Reuse existing functionality from memdup_user() instead of keeping > duplicate source code. > > Generated by: scripts/coccinelle/api/memdup_user.cocci > > Delete local variables which became unnecessary with this refactoring > in two function implementations. > > Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support") This doesn't fix anything and the Fixes: line is not appropriate. > diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c [] > @@ -715,36 +715,16 @@ static int pkey_apqns4keytype(enum pkey_key_type ktype, > > static void *_copy_key_from_user(void __user *ukey, size_t keylen) > { > - void *kkey; > - > - if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE) > - return ERR_PTR(-EINVAL); > - kkey = kmalloc(keylen, GFP_KERNEL); > - if (!kkey) > - return ERR_PTR(-ENOMEM); > - if (copy_from_user(kkey, ukey, keylen)) { > - kfree(kkey); > - return ERR_PTR(-EFAULT); > - } > - > - return kkey; > + return !ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE > + ? ERR_PTR(-EINVAL) > + : memdup_user(ukey, keylen); This is a very poor use of ternary ?: code. This is much more readable for humans. if (!ukey || keylen < MINKEYBLOBSIZE || keylen > KBLOBBUFSIZE) return ERR_PTR(-EINVAL); return memdup_user(ukey, keylen); The compiler will produce the same code. > static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns) > { > - void *kapqns = NULL; > - size_t nbytes; > - > - if (uapqns && nr_apqns > 0) { > - nbytes = nr_apqns * sizeof(struct pkey_apqn); > - kapqns = kmalloc(nbytes, GFP_KERNEL); > - if (!kapqns) > - return ERR_PTR(-ENOMEM); > - if (copy_from_user(kapqns, uapqns, nbytes)) > - return ERR_PTR(-EFAULT); > - } > - > - return kapqns; > + return uapqns && nr_apqns > 0 > + ? memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn)) > + : NULL; And here you reverse the form of the earlier block. Please be consistent and use this style: if (!uapqns || nr_apqns <= 0) return NULL; return memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn));