From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS22989 209.51.188.0/24 X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id C24711F4C1 for ; Thu, 1 Dec 2022 11:53:21 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=clisp.org header.i=@clisp.org header.b="SF2S8gL0"; dkim-atps=neutral Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1p0i7g-0006gN-0W; Thu, 01 Dec 2022 06:53:00 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p0i7e-0006fy-Ma for bug-gnulib@gnu.org; Thu, 01 Dec 2022 06:52:58 -0500 Received: from mo4-p00-ob.smtp.rzone.de ([85.215.255.24]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p0i7b-0004Rw-PH for bug-gnulib@gnu.org; Thu, 01 Dec 2022 06:52:58 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1669895564; s=strato-dkim-0002; d=clisp.org; h=References:In-Reply-To:Message-ID:Date:Subject:To:From:Cc:Date:From: Subject:Sender; bh=TkL1V2p3X7dURp4O7WF7R+BZZ+neGKnQHLAFPGKcAUk=; b=SF2S8gL0VAS0lI6JLNvuQnaND5CGQ12EIL9EYigRXsE3HC7vasz7uwqtuIG8TFMotU dep7OI83RHfmlK99To3PuUtHhFi5rthRJHDcmBbmRvm2aHXs2DZJNYPIylXohsy54a15 5BnZabr46GFN1S7BPkD2QjZYVtQko5VXQlEDmN1ANbn/4Y/BOrO6GxiEzMZDp0nCSqRA 2wRdewiFT4NiGm0WwqHSDRwYNsHpthHYeQoyrldtqCGQ2LSY4ACeJWyQw+QLbZlxwHJE tPjXE9vCXoolQ7OxEW5bKTeGGdnit3dGgRh/6gjc2Zod6k+UvQTsh1SmoLTH5RWr0sDJ m0kg== Authentication-Results: strato.com; dkim=none X-RZG-AUTH: ":Ln4Re0+Ic/6oZXR1YgKryK8brlshOcZlIWs+iCP5vnk6shH0WWb0LN8XZoH94zq68+3cfpPE2faGIYKovsaNk9hwvYMVtbw64A==" X-RZG-CLASS-ID: mo00 Received: from nimes.localnet by smtp.strato.de (RZmta 48.2.1 AUTH) with ESMTPSA id v9c7e6yB1BqiM15 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits)) (Client did not present a certificate); Thu, 1 Dec 2022 12:52:44 +0100 (CET) From: Bruno Haible To: bug-gnulib@gnu.org, Ondrej Valousek Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux Date: Thu, 01 Dec 2022 12:52:43 +0100 Message-ID: <7457692.nlapOpYt14@nimes> In-Reply-To: <20221201095047.1416041-1-ondrej.valousek.xm@renesas.com> References: <20221201095047.1416041-1-ondrej.valousek.xm@renesas.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Received-SPF: none client-ip=85.215.255.24; envelope-from=bruno@clisp.org; helo=mo4-p00-ob.smtp.rzone.de X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_PASS=-0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: bug-gnulib@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gnulib discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Sender: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Hi Ondrej, Ondrej Valousek wrote: > Just sending the modified patch (fixed formatting + few suggestions from = Andreas included) > Does it look OK now? Some formatting issues, still. Details below. > diff --git a/doc/acl-nfsv4.txt b/doc/acl-nfsv4.txt > new file mode 100644 > index 000000000..552a62ca3 > --- /dev/null > +++ b/doc/acl-nfsv4.txt Thanks for adding this file. It will help maintenance. > @@ -0,0 +1,17 @@ > +General introduction: > + https://linux.die.net/man/5/nfs4_acl > + > +The NFSv4 acls are defined in RFC7530 and as such, every NFSv4 server su= pporting ACLs > +will support this kind of ACLs (note the difference from POSIX draft ACL= s) > + > +The ACLs can be obtained via the nfsv4-acl-tools, i.e. > + > +$ nfs4_getfacl > + > +# file: > +A::OWNER@:rwaDxtTnNcCy > +A::GROUP@:rwaDxtTnNcy > +A::EVERYONE@:rwaDxtTnNcy > + > +GNULib is aiming to only provide a basic support of these, i.e. recogniz= e trivial Please spell it 'Gnulib', not 'GNULib'. > +and non-trivial ACLs > diff --git a/lib/acl-internal.c b/lib/acl-internal.c > index be244c67a..86df38854 100644 > --- a/lib/acl-internal.c > +++ b/lib/acl-internal.c > @@ -25,6 +25,9 @@ > =20 > #if USE_ACL && HAVE_ACL_GET_FILE /* Linux, FreeBSD, Mac OS X, IRIX, Tru6= 4, Cygwin >=3D 2.5 */ > =20 > +#include > +# include We try to keep a visible indentation level for preprocessor directives/ statements/instructions. The indentation level is indicated by spaces after the '#'. So, here please insert a space before 'include '. > + > # if HAVE_ACL_TYPE_EXTENDED /* Mac OS X */ > =20 > /* ACL is an ACL, from a file, stored as type ACL_TYPE_EXTENDED. > @@ -122,6 +125,98 @@ acl_default_nontrivial (acl_t acl) > return (acl_entries (acl) > 0); > } > =20 > +#define ACE4_WHO_OWNER "OWNER@" > +#define ACE4_WHO_GROUP "GROUP@" > +#define ACE4_WHO_EVERYONE "EVERYONE@" > + > +#define ACE4_ACCESS_ALLOWED_ACE_TYPE 0 > +#define ACE4_ACCESS_DENIED_ACE_TYPE 1 In these preprocessor statements, we are at preprocessor indentation level 2. So please change '#define' to '# define' here. > +/* ACE flag values */ What does this comment belong to? The two lines above or the line below? If it's the lines above, then the comment should be moved up. If it's the line below, a blank line should be removed. > + > +#define ACE4_IDENTIFIER_GROUP 0x00000040 > +#define ROUNDUP(x, y) (((x) + (y) - 1) & - (y)) > + > +int > +acl_nfs4_nontrivial (char *xattr, int len) > +{ > + int ret =3D 0,wholen,bufs =3D len; Please define one variable per line. When reading the code, we want to see quickly where a certain variable is declared, and that is significantly harder if multiple variables are declared on the same line, especially with initializations. > + uint32_t flag,type,num_aces =3D ntohl(*((uint32_t*)(xattr))); /* Grab = the number of aces in the acl */ Likewise. Also, please add a space after 'ntohl' because it's a function ca= ll. > + char *bufp =3D xattr; > + int num_a_aces =3D 0, num_d_aces =3D 0; Please define one variable per line. > + ret =3D 0; > + bufp +=3D 4; /* sizeof(uint32_t); */ > + bufs -=3D 4; > + > + for(uint32_t ace_n =3D 0; num_aces > ace_n ; ace_n++) { Please add a space after 'for'. Also put the opening brace on a separate li= ne. > + int d_ptr; > + > + /* Get the acl type */ > + if(bufs <=3D 0)=20 Please add a space after 'if'. > + return -1; > + > + type =3D ntohl(*((uint32_t*)bufp)); Please add a space after 'ntohl' because it's a function call. > + bufp +=3D 4; > + bufs -=3D 4; > + if (bufs <=3D 0) > + return -1; > + > + flag =3D ntohl(*((uint32_t*)bufp)); Likewise. > + /* As per RFC 7530, the flag should be 0, but we are just generous t= o Netapp > + * and also accept the Group flag > + */ > + if (flag & ~ACE4_IDENTIFIER_GROUP) > + return 1; > + > + /* we skip mask -=20 > + * it's too risky to test it and it does not seem to be actually nee= ded */ > + bufp +=3D 2*4; > + bufs -=3D 2*4; > + > + if (bufs <=3D 0)=20 > + return -1; > + > + wholen =3D ntohl(*((uint32_t*)bufp)); Likewise. > + > + bufp +=3D 4; > + bufs -=3D 4; > + > + /* Get the who string */ > + if (bufs <=3D 0) > + return -1; > + > + /* for trivial ACL, we expect max 5 (typically 3) ACES, 3 Allow, 2 d= eny */=20 > + if (((strncmp(bufp,ACE4_WHO_OWNER,wholen) =3D=3D 0) || Please move the operator || to the next line. This allows reading the multi= =2Dline expressions faster, just by looking at the first few characters of each lin= e. > + (strncmp(bufp,ACE4_WHO_GROUP,wholen) =3D=3D 0)) && Likewise, please move the operator && to the next line. > + wholen =3D=3D 6 ) You can remove the space before the closing parenthesis. > + { > + if(type =3D=3D ACE4_ACCESS_ALLOWED_ACE_TYPE) Please add a space after 'if'. > + num_a_aces++; > + if(type =3D=3D ACE4_ACCESS_DENIED_ACE_TYPE) Likewise. > + num_d_aces++; > + } else=20 Please put the closing brace and the 'else' on separate lines. > + if ((strncmp(bufp,ACE4_WHO_EVERYONE,wholen) =3D=3D 0) && Please move the operator && to the next line. > + (type =3D=3D ACE4_ACCESS_ALLOWED_ACE_TYPE) && Likewise. > + (wholen =3D=3D 9)) > + num_a_aces++; > + else > + return 1; > + > + d_ptr =3D ROUNDUP(wholen,4); Please add a space after ROUNDUP, since it's an invocation of a function-li= ke macro. > + bufp +=3D d_ptr; > + bufs -=3D d_ptr; > + > + /* Make sure we aren't outside our domain */ > + if (bufs < 0) > + return -1; > + > + } > + return (num_a_aces <=3D 3) && (num_d_aces <=3D 2) && Please move the operator && to the next line. > + ( num_a_aces + num_d_aces =3D=3D num_aces) ? 0 : 1; You can remove the space after the opening parenthesis. > + > +} > + > # endif > =20 > #elif USE_ACL && HAVE_FACL && defined GETACL /* Solaris, Cygwin < 2.5, n= ot HP-UX */ > diff --git a/lib/acl-internal.h b/lib/acl-internal.h > index 94553fab2..88f1d8f1d 100644 > --- a/lib/acl-internal.h > +++ b/lib/acl-internal.h > @@ -146,6 +146,9 @@ rpl_acl_set_fd (int fd, acl_t acl) > # define acl_entries rpl_acl_entries > extern int acl_entries (acl_t); > # endif > +/* Return 1 if given ACL in XDR format is non-trivial=20 > + * Return 0 if it is trivial */ > +extern int acl_nfs4_nontrivial (char *,int); > =20 > # if HAVE_ACL_TYPE_EXTENDED /* Mac OS X */ > /* ACL is an ACL, from a file, stored as type ACL_TYPE_EXTENDED. > diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c > index e02f0626a..8e43dd70f 100644 > --- a/lib/file-has-acl.c > +++ b/lib/file-has-acl.c > @@ -32,6 +32,11 @@ > #if GETXATTR_WITH_POSIX_ACLS > # include > # include > +# include > +#ifndef XATTR_NAME_NFSV4_ACL > +#define XATTR_NAME_NFSV4_ACL "system.nfs4_acl" > +#endif > +#define TRIVIAL_NFS4_ACL_MAX_LENGTH 128 Please add spaces after '#', to indicate the preprocessor statement indentation level. > #endif > =20 > /* Return 1 if NAME has a nontrivial access control list, > @@ -67,6 +72,22 @@ file_has_acl (char const *name, struct stat const *sb) > return 1; > } > =20 > + if (ret < 0) > + { /* we might be on NFS, so try to check NFSv4 ACLs too */ > + char xattr[TRIVIAL_NFS4_ACL_MAX_LENGTH]; > + > + errno =3D 0; /* we need to reset errno set by the previous get= xattr() */ > + ret =3D getxattr (name, XATTR_NAME_NFSV4_ACL, xattr, TRIVIAL_N= =46S4_ACL_MAX_LENGTH); > + if (ret < 0 && errno =3D=3D ENODATA) > + ret =3D 0; > + else > + if (ret < 0 && errno =3D=3D ERANGE) > + return 1; /* we won't fit into the buffer, so non-trivial= ACL is presented */ > + else This 'else' statement and the following lines should be indented by 2 more spaces. > + if (ret > 0) > + return acl_nfs4_nontrivial(xattr,ret); Please add spaces before the opening parenthesis and after the comma. > + /* looks like trivial ACL, but we need to investigate furt= her */ It looks like this comment refers to the previous line? Then move it up one= line. > + } > if (ret < 0) > return - acl_errno_valid (errno); > return ret; > diff --git a/m4/acl.m4 b/m4/acl.m4 > index 8909442d7..9445edf9c 100644 > --- a/m4/acl.m4 > +++ b/m4/acl.m4 > @@ -197,7 +197,10 @@ AC_DEFUN([gl_FILE_HAS_ACL], > [gl_cv_getxattr_with_posix_acls=3Dyes])]) > fi > if test "$gl_cv_getxattr_with_posix_acls" =3D yes; then > - LIB_HAS_ACL=3D > + dnl We need to pull libacl back to make linker happy, but strictly > + dnl speaking, we do not need it > + LIB_HAS_ACL=3D$LIB_ACL > + gl_need_lib_has_acl=3D1 > AC_DEFINE([GETXATTR_WITH_POSIX_ACLS], 1, > [Define to 1 if getxattr works with XATTR_NAME_POSIX_ACL_ACCESS > and XATTR_NAME_POSIX_ACL_DEFAULT.]) >=20 I see nothing in the changes above that needs to pull in libacl. Therefore please leave the change to m4/acl.m4 out for the moment. We'll need to disc= uss this part in a separate thread; as first part of the thread please describe the problem (link error?) that you are having =E2=80=94 since Andreas said = that he doesn't get a link error, and I haven't seen a link error regarding libacl in years either. Thanks! Bruno