From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753559AbcFBQxX (ORCPT ); Thu, 2 Jun 2016 12:53:23 -0400 Received: from e38.co.us.ibm.com ([32.97.110.159]:32843 "EHLO e38.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751369AbcFBQxV (ORCPT ); Thu, 2 Jun 2016 12:53:21 -0400 X-IBM-Helo: d03dlp01.boulder.ibm.com X-IBM-MailFrom: rui.teng@linux.vnet.ibm.com X-IBM-RcptTo: luto@amacapital.net;serge.hallyn@canonical.com;serge@hallyn.com;james.l.morris@oracle.com;linux-kernel@vger.kernel.org;linux-security-module@vger.kernel.org Subject: Re: [PATCH] security: Use || instead of | for boolean expressions To: "Serge E. Hallyn" References: <1464760982-3721-1-git-send-email-rui.teng@linux.vnet.ibm.com> <20160602141349.GA26954@mail.hallyn.com> Cc: serge.hallyn@canonical.com, james.l.morris@oracle.com, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Andy Lutomirski From: Rui Teng Message-ID: Date: Fri, 3 Jun 2016 00:53:07 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <20160602141349.GA26954@mail.hallyn.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16060216-0029-0000-0000-00002C455FE9 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 6/2/16 10:13 PM, Serge E. Hallyn wrote: > On Wed, Jun 01, 2016 at 02:03:02PM +0800, Rui Teng wrote: >> Sparse spits out the following warning: >> security/commoncap.c:989:41: warning: dubious: !x | y >> >> Bitwise and logical are equivalent here, but logical was intended. >> Replacing the bit-wise '|' with the boolean '||' silences the sparse warning. > > Hi, > > this looks ok, but I'm worried by > >> The generated code for both cases is the same. > > That cannot be. The logical result should be the same, but the > generated code cannot be. Thanks for cc:ing the author. I tried to write a sample code to verify it before. Both || and | will generate the same assembly code. For example, compiling following code with "gcc -O2 -S main.c", and replacing || with | can generate the same assembly code. - main.c ------------ int parse(int a, int b, int c) { if (a || b || c) return 1; else return 0; } Of cause, it is only a sample on x86, but even if the generated code is not the same, the logical will be better than bitwise. Because (a || b || c) means (a != 0 || b != 0 || c != 0), once a != 0, the whole expression will be true(short-circuit evaluation). and (a | b | c) means calculate the bitwise first and check the result in the end. And since the args are all integer, there is no need to avoid any short-circuit. > > I'm cc:ing Andy as this code came in with his patch. Is there an > actual reason for having used bitwise here? > > thanks, > -serge > >> Signed-off-by: Rui Teng >> --- >> security/commoncap.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/security/commoncap.c b/security/commoncap.c >> index e7fadde..8f6fb24 100644 >> --- a/security/commoncap.c >> +++ b/security/commoncap.c >> @@ -976,7 +976,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, >> >> case PR_CAP_AMBIENT: >> if (arg2 == PR_CAP_AMBIENT_CLEAR_ALL) { >> - if (arg3 | arg4 | arg5) >> + if (arg3 || arg4 || arg5) >> return -EINVAL; >> >> new = prepare_creds(); >> @@ -986,7 +986,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, >> return commit_creds(new); >> } >> >> - if (((!cap_valid(arg3)) | arg4 | arg5)) >> + if (((!cap_valid(arg3)) || arg4 || arg5)) >> return -EINVAL; >> >> if (arg2 == PR_CAP_AMBIENT_IS_SET) { >> -- >> 2.7.4 >