Some functions that are declared when CONFIG_POSIX_ACL is defined
are not declared when CONFIG_POSIX_ACL is not defined. Add the
missing ones:
set_posix_acl(), posix_acl_update_mode(), get_cached_acl(),
get_cached_acl_rcu(), set_cached_acl(), forget_cached_acl().
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
include/linux/posix_acl.h | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index 90797f1b421d..f6d206359da5 100644
--- a/include/linux/posix_acl.h
+++ b/include/linux/posix_acl.h
@@ -117,6 +117,39 @@ static inline int posix_acl_create(struct inode *inode, umode_t *mode,
static inline void forget_all_cached_acls(struct inode *inode)
{
}
+
+static inline int set_posix_acl(struct inode *inode, int type,
+ struct posix_acl *acl)
+{
+ return 0;
+}
+
+static inline int posix_acl_update_mode(struct inode *, umode_t *,
+ struct posix_acl **)
+{
+ return 0;
+}
+
+static inline struct posix_acl *get_cached_acl(struct inode *inode,
+ int type)
+{
+ return NULL;
+}
+
+static inline struct posix_acl *get_cached_acl_rcu(struct inode *inode,
+ int type)
+{
+ return NULL;
+}
+
+static inline void set_cached_acl(struct inode *inode, int type,
+ struct posix_acl *acl)
+{
+}
+
+static inline void forget_cached_acl(struct inode *inode, int type)
+{
+}
#endif /* CONFIG_FS_POSIX_ACL */
struct posix_acl *get_acl(struct inode *inode, int type);
--
2.29.2
On 11/29/20 5:44 PM, Sergey Senozhatsky wrote:
> Some functions that are declared when CONFIG_POSIX_ACL is defined
> are not declared when CONFIG_POSIX_ACL is not defined. Add the
> missing ones:
> set_posix_acl(), posix_acl_update_mode(), get_cached_acl(),
> get_cached_acl_rcu(), set_cached_acl(), forget_cached_acl().
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
Hi,
I can't find CONFIG_POSIX_ACL in the kernel source tree.
Should it be CONFIG_FS_POSIX_ACL ?
How did you test this?
> ---
> include/linux/posix_acl.h | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
> index 90797f1b421d..f6d206359da5 100644
> --- a/include/linux/posix_acl.h
> +++ b/include/linux/posix_acl.h
> @@ -117,6 +117,39 @@ static inline int posix_acl_create(struct inode *inode, umode_t *mode,
> static inline void forget_all_cached_acls(struct inode *inode)
> {
> }
> +
> +static inline int set_posix_acl(struct inode *inode, int type,
> + struct posix_acl *acl)
> +{
> + return 0;
> +}
> +
> +static inline int posix_acl_update_mode(struct inode *, umode_t *,
> + struct posix_acl **)
> +{
> + return 0;
> +}
> +
> +static inline struct posix_acl *get_cached_acl(struct inode *inode,
> + int type)
> +{
> + return NULL;
> +}
> +
> +static inline struct posix_acl *get_cached_acl_rcu(struct inode *inode,
> + int type)
> +{
> + return NULL;
> +}
> +
> +static inline void set_cached_acl(struct inode *inode, int type,
> + struct posix_acl *acl)
> +{
> +}
> +
> +static inline void forget_cached_acl(struct inode *inode, int type)
> +{
> +}
> #endif /* CONFIG_FS_POSIX_ACL */
>
> struct posix_acl *get_acl(struct inode *inode, int type);
>
thanks.
--
~Randy
On (20/11/29 18:00), Randy Dunlap wrote:
> On 11/29/20 5:44 PM, Sergey Senozhatsky wrote:
> > Some functions that are declared when CONFIG_POSIX_ACL is defined
> > are not declared when CONFIG_POSIX_ACL is not defined. Add the
> > missing ones:
> > set_posix_acl(), posix_acl_update_mode(), get_cached_acl(),
> > get_cached_acl_rcu(), set_cached_acl(), forget_cached_acl().
> >
> > Signed-off-by: Sergey Senozhatsky <[email protected]>
>
> Hi,
>
> I can't find CONFIG_POSIX_ACL in the kernel source tree.
> Should it be CONFIG_FS_POSIX_ACL ?
Oh, yes, CONFIG_POSIX_ACL. My bad.
> How did you test this?
You know what - scratch this patch. Sorry for the noise.
Some of the posix_acl.h functions are guarded by ifdef/ifndef
CONFIG_FS_POSIX_ACL, and some are not. This can break the build
if the code in question doesn't use ifdef CONFIG_FS_POSIX_ACL
(which happens with our code). But this patch is not enough,
apparently, we need to add ifdef CONFIG_FS_POSIX_ACL to our
code anyway, because of, for instance, posix_acl_alloc() which
is undefined for !FS_POSIX_ACL builds. Sorry for the noise.
-ss
On (20/11/30 12:15), Sergey Senozhatsky wrote:
> On (20/11/29 18:00), Randy Dunlap wrote:
> > On 11/29/20 5:44 PM, Sergey Senozhatsky wrote:
> > > Some functions that are declared when CONFIG_POSIX_ACL is defined
> > > are not declared when CONFIG_POSIX_ACL is not defined. Add the
> > > missing ones:
> > > set_posix_acl(), posix_acl_update_mode(), get_cached_acl(),
> > > get_cached_acl_rcu(), set_cached_acl(), forget_cached_acl().
> > >
> > > Signed-off-by: Sergey Senozhatsky <[email protected]>
> >
> > Hi,
> >
> > I can't find CONFIG_POSIX_ACL in the kernel source tree.
> > Should it be CONFIG_FS_POSIX_ACL ?
>
> Oh, yes, CONFIG_POSIX_ACL. My bad.
... CONFIG_FS_POSIX_ACL. I did it again.
-ss
A quick question, shouldn't there be dummy definitions for
the EXPORT_SYMBOL-s? So that external modules can be modprobed
and used.
Some of posix_acl exported symbols have dummy definitions,
others don't.
E.g. posix_acl_create() is exported symbol and it's defined for
both FS_POSIX_ACL and !FS_POSIX_ACL. While exported set_posix_acl()
is defined only for FS_POSIX_ACL config.
---
diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index 90797f1b421d..8a6c77a69761 100644
--- a/include/linux/posix_acl.h
+++ b/include/linux/posix_acl.h
@@ -59,19 +59,19 @@ posix_acl_release(struct posix_acl *acl)
/* posix_acl.c */
+extern int posix_acl_permission(struct inode *, const struct posix_acl *, int);
+
+extern struct posix_acl *get_posix_acl(struct inode *, int);
+
+#ifdef CONFIG_FS_POSIX_ACL
extern void posix_acl_init(struct posix_acl *, int);
extern struct posix_acl *posix_acl_alloc(int, gfp_t);
-extern int posix_acl_valid(struct user_namespace *, const struct posix_acl *);
-extern int posix_acl_permission(struct inode *, const struct posix_acl *, int);
-extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t);
extern int posix_acl_equiv_mode(const struct posix_acl *, umode_t *);
+extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t);
extern int __posix_acl_create(struct posix_acl **, gfp_t, umode_t *);
extern int __posix_acl_chmod(struct posix_acl **, gfp_t, umode_t);
-
-extern struct posix_acl *get_posix_acl(struct inode *, int);
extern int set_posix_acl(struct inode *, int, struct posix_acl *);
-
-#ifdef CONFIG_FS_POSIX_ACL
+extern int posix_acl_valid(struct user_namespace *, const struct posix_acl *);
extern int posix_acl_chmod(struct inode *, umode_t);
extern int posix_acl_create(struct inode *, umode_t *, struct posix_acl **,
struct posix_acl **);
@@ -91,18 +91,61 @@ static inline void cache_no_acl(struct inode *inode)
inode->i_acl = NULL;
inode->i_default_acl = NULL;
}
+
+struct posix_acl *get_acl(struct inode *inode, int type);
#else
+static inline void posix_acl_init(struct posix_acl *, int)
+{
+}
+
+static inline struct posix_acl *posix_acl_alloc(int, gfp_t)
+{
+ return NULL;
+}
+
+static inline int posix_acl_valid(struct user_namespace *,
+ const struct posix_acl *)
+{
+ return 0;
+}
+
+static inline int posix_acl_equiv_mode(const struct posix_acl *, umode_t *)
+{
+ return 0;
+}
+
+static inline struct posix_acl *posix_acl_from_mode(umode_t, gfp_t)
+{
+ return NULL;
+}
+
static inline int posix_acl_chmod(struct inode *inode, umode_t mode)
{
return 0;
}
+static inline int set_posix_acl(struct inode *, int, struct posix_acl *)
+{
+ return 0;
+}
+
#define simple_set_acl NULL
static inline int simple_acl_create(struct inode *dir, struct inode *inode)
{
return 0;
}
+
+static inline int __posix_acl_create(struct posix_acl **, gfp_t, umode_t *)
+{
+ return 0;
+}
+
+static inline int __posix_acl_chmod(struct posix_acl **, gfp_t, umode_t)
+{
+ return 0;
+}
+
static inline void cache_no_acl(struct inode *inode)
{
}
@@ -117,8 +160,38 @@ static inline int posix_acl_create(struct inode *inode, umode_t *mode,
static inline void forget_all_cached_acls(struct inode *inode)
{
}
+
+static inline struct posix_acl *get_cached_acl(struct inode *inode, int type)
+{
+ return NULL;
+}
+
+static inline struct posix_acl *get_cached_acl_rcu(struct inode *inode,
+ int type)
+{
+ return NULL;
+}
+
+static inline void set_cached_acl(struct inode *inode, int type,
+ struct posix_acl *acl)
+{
+}
+
+static inline void forget_cached_acl(struct inode *inode, int type)
+{
+}
+
+static inline struct posix_acl *get_acl(struct inode *inode, int type)
+{
+ return NULL;
+}
+
+static inline int posix_acl_update_mode(struct inode *, umode_t *,
+ struct posix_acl **)
+{
+ return 0;
+}
#endif /* CONFIG_FS_POSIX_ACL */
-struct posix_acl *get_acl(struct inode *inode, int type);
#endif /* __LINUX_POSIX_ACL_H */
On 11/29/20 7:37 PM, Sergey Senozhatsky wrote:
> A quick question, shouldn't there be dummy definitions for
> the EXPORT_SYMBOL-s? So that external modules can be modprobed
> and used.
>
> Some of posix_acl exported symbols have dummy definitions,
> others don't.
>
> E.g. posix_acl_create() is exported symbol and it's defined for
> both FS_POSIX_ACL and !FS_POSIX_ACL. While exported set_posix_acl()
> is defined only for FS_POSIX_ACL config.
Hi,
Currently CONFIG_FS_POSIX_ACL differences seem to be handled in
each source file as needed:
fs/inode.c:#ifdef CONFIG_FS_POSIX_ACL
fs/inode.c:#ifdef CONFIG_FS_POSIX_ACL
fs/namei.c:#ifdef CONFIG_FS_POSIX_ACL
fs/overlayfs/dir.c: if (!IS_ENABLED(CONFIG_FS_POSIX_ACL) || !acl)
fs/overlayfs/inode.c: if (!IS_ENABLED(CONFIG_FS_POSIX_ACL) || !IS_POSIXACL(realinode))
fs/overlayfs/inode.c:#ifdef CONFIG_FS_POSIX_ACL
fs/overlayfs/super.c:#ifdef CONFIG_FS_POSIX_ACL
fs/xattr.c:#ifdef CONFIG_FS_POSIX_ACL
include/linux/evm.h:#ifdef CONFIG_FS_POSIX_ACL
include/linux/fs.h:#ifdef CONFIG_FS_POSIX_ACL
include/linux/posix_acl.h:#ifdef CONFIG_FS_POSIX_ACL
include/linux/posix_acl.h:#endif /* CONFIG_FS_POSIX_ACL */
include/linux/posix_acl_xattr.h:#ifdef CONFIG_FS_POSIX_ACL
However, I have no objection to your patch.
I am adding Andreas & Al for their viewpoints.
> ---
>
> diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
> index 90797f1b421d..8a6c77a69761 100644
> --- a/include/linux/posix_acl.h
> +++ b/include/linux/posix_acl.h
> @@ -59,19 +59,19 @@ posix_acl_release(struct posix_acl *acl)
>
> /* posix_acl.c */
>
> +extern int posix_acl_permission(struct inode *, const struct posix_acl *, int);
> +
> +extern struct posix_acl *get_posix_acl(struct inode *, int);
> +
> +#ifdef CONFIG_FS_POSIX_ACL
> extern void posix_acl_init(struct posix_acl *, int);
> extern struct posix_acl *posix_acl_alloc(int, gfp_t);
> -extern int posix_acl_valid(struct user_namespace *, const struct posix_acl *);
> -extern int posix_acl_permission(struct inode *, const struct posix_acl *, int);
> -extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t);
> extern int posix_acl_equiv_mode(const struct posix_acl *, umode_t *);
> +extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t);
> extern int __posix_acl_create(struct posix_acl **, gfp_t, umode_t *);
> extern int __posix_acl_chmod(struct posix_acl **, gfp_t, umode_t);
> -
> -extern struct posix_acl *get_posix_acl(struct inode *, int);
> extern int set_posix_acl(struct inode *, int, struct posix_acl *);
> -
> -#ifdef CONFIG_FS_POSIX_ACL
> +extern int posix_acl_valid(struct user_namespace *, const struct posix_acl *);
> extern int posix_acl_chmod(struct inode *, umode_t);
> extern int posix_acl_create(struct inode *, umode_t *, struct posix_acl **,
> struct posix_acl **);
> @@ -91,18 +91,61 @@ static inline void cache_no_acl(struct inode *inode)
> inode->i_acl = NULL;
> inode->i_default_acl = NULL;
> }
> +
> +struct posix_acl *get_acl(struct inode *inode, int type);
> #else
> +static inline void posix_acl_init(struct posix_acl *, int)
> +{
> +}
> +
> +static inline struct posix_acl *posix_acl_alloc(int, gfp_t)
> +{
> + return NULL;
> +}
> +
> +static inline int posix_acl_valid(struct user_namespace *,
> + const struct posix_acl *)
> +{
> + return 0;
> +}
> +
> +static inline int posix_acl_equiv_mode(const struct posix_acl *, umode_t *)
> +{
> + return 0;
> +}
> +
> +static inline struct posix_acl *posix_acl_from_mode(umode_t, gfp_t)
> +{
> + return NULL;
> +}
> +
> static inline int posix_acl_chmod(struct inode *inode, umode_t mode)
> {
> return 0;
> }
>
> +static inline int set_posix_acl(struct inode *, int, struct posix_acl *)
> +{
> + return 0;
> +}
> +
> #define simple_set_acl NULL
>
> static inline int simple_acl_create(struct inode *dir, struct inode *inode)
> {
> return 0;
> }
> +
> +static inline int __posix_acl_create(struct posix_acl **, gfp_t, umode_t *)
> +{
> + return 0;
> +}
> +
> +static inline int __posix_acl_chmod(struct posix_acl **, gfp_t, umode_t)
> +{
> + return 0;
> +}
> +
> static inline void cache_no_acl(struct inode *inode)
> {
> }
> @@ -117,8 +160,38 @@ static inline int posix_acl_create(struct inode *inode, umode_t *mode,
> static inline void forget_all_cached_acls(struct inode *inode)
> {
> }
> +
> +static inline struct posix_acl *get_cached_acl(struct inode *inode, int type)
> +{
> + return NULL;
> +}
> +
> +static inline struct posix_acl *get_cached_acl_rcu(struct inode *inode,
> + int type)
> +{
> + return NULL;
> +}
> +
> +static inline void set_cached_acl(struct inode *inode, int type,
> + struct posix_acl *acl)
> +{
> +}
> +
> +static inline void forget_cached_acl(struct inode *inode, int type)
> +{
> +}
> +
> +static inline struct posix_acl *get_acl(struct inode *inode, int type)
> +{
> + return NULL;
> +}
> +
> +static inline int posix_acl_update_mode(struct inode *, umode_t *,
> + struct posix_acl **)
> +{
> + return 0;
> +}
> #endif /* CONFIG_FS_POSIX_ACL */
>
> -struct posix_acl *get_acl(struct inode *inode, int type);
>
> #endif /* __LINUX_POSIX_ACL_H */
>
--
~Randy
Hi Sergey,
I love your patch! Perhaps something to improve:
[auto build test WARNING on hch-configfs/for-next]
[also build test WARNING on linux/master linus/master v5.10-rc6 next-20201127]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sergey-Senozhatsky/posix_acl-h-define-missing-ACL-functions-on-non-posix-acl-build/20201130-095018
base: git://git.infradead.org/users/hch/configfs.git for-next
config: s390-randconfig-r005-20201130 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project dfcf1acf13226be0f599a7ab6b395b66dc9545d6)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/0day-ci/linux/commit/377ebc564d08d21a4bc83fecc7f1238e342823d1
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sergey-Senozhatsky/posix_acl-h-define-missing-ACL-functions-on-non-posix-acl-build/20201130-095018
git checkout 377ebc564d08d21a4bc83fecc7f1238e342823d1
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from fs/overlayfs/super.c:16:
In file included from include/linux/posix_acl_xattr.h:15:
include/linux/posix_acl.h:121:19: error: static declaration of 'set_posix_acl' follows non-static declaration
static inline int set_posix_acl(struct inode *inode, int type,
^
include/linux/posix_acl.h:72:12: note: previous declaration is here
extern int set_posix_acl(struct inode *, int, struct posix_acl *);
^
>> include/linux/posix_acl.h:127:55: warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions]
static inline int posix_acl_update_mode(struct inode *, umode_t *,
^
include/linux/posix_acl.h:127:66: warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions]
static inline int posix_acl_update_mode(struct inode *, umode_t *,
^
include/linux/posix_acl.h:128:25: warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions]
struct posix_acl **)
^
3 warnings and 1 error generated.
vim +127 include/linux/posix_acl.h
126
> 127 static inline int posix_acl_update_mode(struct inode *, umode_t *,
128 struct posix_acl **)
129 {
130 return 0;
131 }
132
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Mon, Nov 30, 2020 at 5:29 AM Randy Dunlap <[email protected]> wrote:
> On 11/29/20 7:37 PM, Sergey Senozhatsky wrote:
> > A quick question, shouldn't there be dummy definitions for
> > the EXPORT_SYMBOL-s? So that external modules can be modprobed
> > and used.
> >
> > Some of posix_acl exported symbols have dummy definitions,
> > others don't.
> >
> > E.g. posix_acl_create() is exported symbol and it's defined for
> > both FS_POSIX_ACL and !FS_POSIX_ACL. While exported set_posix_acl()
> > is defined only for FS_POSIX_ACL config.
This is to keep the amount of ifdefs in the code reasonably low: by
defining posix_acl_create as a dummy inline function like that, inode
creation in filesystems can be implemented without any ifdefs as in
jffs2_init_acl_pre whether or not CONFIG_FS_POSIX_ACL is enabled, for
example. Have a look at different filesystems to see how they avoid
using POSIX ACL code when that feature is disabled.
Note that ext2 / ext4 could be built without POSIX ACL support in the
past. That's at least broken since the following two commits though:
commit 59fed3bf8a461 ("ext2: cache NULL when both default_acl and
acl are NULL")
commit 6fd941784b8ac ("ext4: cache NULL when both default_acl and
acl are NULL")
> Hi,
>
> Currently CONFIG_FS_POSIX_ACL differences seem to be handled in
> each source file as needed:
>
> fs/inode.c:#ifdef CONFIG_FS_POSIX_ACL
> fs/inode.c:#ifdef CONFIG_FS_POSIX_ACL
> fs/namei.c:#ifdef CONFIG_FS_POSIX_ACL
> fs/overlayfs/dir.c: if (!IS_ENABLED(CONFIG_FS_POSIX_ACL) || !acl)
> fs/overlayfs/inode.c: if (!IS_ENABLED(CONFIG_FS_POSIX_ACL) || !IS_POSIXACL(realinode))
> fs/overlayfs/inode.c:#ifdef CONFIG_FS_POSIX_ACL
> fs/overlayfs/super.c:#ifdef CONFIG_FS_POSIX_ACL
> fs/xattr.c:#ifdef CONFIG_FS_POSIX_ACL
> include/linux/evm.h:#ifdef CONFIG_FS_POSIX_ACL
> include/linux/fs.h:#ifdef CONFIG_FS_POSIX_ACL
> include/linux/posix_acl.h:#ifdef CONFIG_FS_POSIX_ACL
> include/linux/posix_acl.h:#endif /* CONFIG_FS_POSIX_ACL */
> include/linux/posix_acl_xattr.h:#ifdef CONFIG_FS_POSIX_ACL
>
> However, I have no objection to your patch.
>
> I am adding Andreas & Al for their viewpoints.
Sergey, what actual problem is your patch trying to solve? It sounds
like this is either theoretical and pointless, or you're trying to
build an external module that uses POSIX ACL functions that shouldn't
be needed when CONFIG_FS_POSIX_ACL is disabled. In the latter case,
the external module will just end up including dead code, so the
module should be fixed instead.
Thanks,
Andreas
On Mon, Nov 30, 2020 at 2:09 PM Andreas Gruenbacher <[email protected]> wrote:
> Note that ext2 / ext4 could be built without POSIX ACL support in the
> past. That's at least broken since the following two commits though:
>
> commit 59fed3bf8a461 ("ext2: cache NULL when both default_acl and
> acl are NULL")
> commit 6fd941784b8ac ("ext4: cache NULL when both default_acl and
> acl are NULL")
Scratch that, this is in fs/ext[24]/acl.c which is only included when
CONFIG_FS_POSIX_ACL is defined.
Thanks,
Andreas
Hi,
On (20/11/30 14:09), Andreas Gruenbacher wrote:
>
> Sergey, what actual problem is your patch trying to solve? It sounds
> like this is either theoretical and pointless, or you're trying to
> build an external module that uses POSIX ACL functions that shouldn't
> be needed when CONFIG_FS_POSIX_ACL is disabled.
It's an external module, that OpenWRT folks build with !FS_POSIX_ACL.
It compiles just fine, but modprobe fails because there are several
exported ACL symbols that don't provide dummy definitions (which the
module in question didn't guard with ifdef-s).
> In the latter case, the external module will just end up including dead
> code, so the module should be fixed instead.
ifdef-s work. But since posix_acl.h already provides some dummy
definitions for exported symbols, I thought that that list can
be extended (become complete).
-ss