* [PATCH v2 0/2] overlayfs: debugging check for valid superblock
@ 2023-05-20 18:41 Andrea Righi
2023-05-20 18:41 ` [PATCH v2 1/2] ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG Andrea Righi
2023-05-20 18:41 ` [PATCH v2 2/2] ovl: make consistent use of OVL_FS() Andrea Righi
0 siblings, 2 replies; 6+ messages in thread
From: Andrea Righi @ 2023-05-20 18:41 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Amir Goldstein, linux-unionfs, linux-kernel
OVL_FS() is used to get a struct ovl_fs from a sturct super_block, but
we don't have any check to determine if the superblock is valid or not.
This can lead to unexpected behaviors or bugs that are pretty hard to
track down.
Add an explicit WARN_ON_ONCE() check to OVL_FS() to make sure it's
always used with a valid overlayfs superblock.
To avoid enabling this additional pendatic check everywhere, introduce
the new config option CONFIG_OVERLAY_FS_DEBUG, that can be used in the
future also for other additional debugging checks.
Maybe a nicer solution could be to return an error from OVL_FS() when
it's used with an invalid superblock and propagate the error in the rest
of overlayfs code, but for now having at least the possibility to
trigger a warning can help to catch potential bugs in advance.
Changelog (v1 -> v2):
- replace BUG_ON() with WARN_ON_ONCE()
- introduce CONFIG_OVERLAY_FS_DEBUG
Andrea Righi (2):
ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG
ovl: make consistent use of OVL_FS()
fs/overlayfs/Kconfig | 9 +++++++++
fs/overlayfs/copy_up.c | 2 +-
fs/overlayfs/export.c | 10 +++++-----
fs/overlayfs/inode.c | 8 ++++----
fs/overlayfs/namei.c | 2 +-
fs/overlayfs/ovl_entry.h | 16 ++++++++++++++++
fs/overlayfs/super.c | 12 ++++++------
fs/overlayfs/util.c | 18 +++++++++---------
8 files changed, 51 insertions(+), 26 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG
2023-05-20 18:41 [PATCH v2 0/2] overlayfs: debugging check for valid superblock Andrea Righi
@ 2023-05-20 18:41 ` Andrea Righi
2023-05-21 6:36 ` Amir Goldstein
2023-05-20 18:41 ` [PATCH v2 2/2] ovl: make consistent use of OVL_FS() Andrea Righi
1 sibling, 1 reply; 6+ messages in thread
From: Andrea Righi @ 2023-05-20 18:41 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Amir Goldstein, linux-unionfs, linux-kernel
Provide a Kconfig option to enable extra debugging checks for overlayfs.
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---
fs/overlayfs/Kconfig | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index 6708e54b0e30..fec5020c3495 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -124,3 +124,12 @@ config OVERLAY_FS_METACOPY
that doesn't support this feature will have unexpected results.
If unsure, say N.
+
+config OVERLAY_FS_DEBUG
+ bool "Overlayfs: turn on extra debugging checks"
+ default n
+ depends on OVERLAY_FS
+ help
+ Say Y here to enable extra debugging checks in overlayfs.
+
+ If unsure, say N.
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] ovl: make consistent use of OVL_FS()
2023-05-20 18:41 [PATCH v2 0/2] overlayfs: debugging check for valid superblock Andrea Righi
2023-05-20 18:41 ` [PATCH v2 1/2] ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG Andrea Righi
@ 2023-05-20 18:41 ` Andrea Righi
2023-05-21 6:51 ` Amir Goldstein
1 sibling, 1 reply; 6+ messages in thread
From: Andrea Righi @ 2023-05-20 18:41 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Amir Goldstein, linux-unionfs, linux-kernel
Always use OVL_FS() to retrieve the corresponding struct ovl_fs from a
struct super_block.
Moreover, make sure that it is exclusively used with an overlayfs
superblock when CONFIG_OVERLAY_FS_DEBUG is enabled (otherwise trigger a
WARN_ON_ONCE).
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---
fs/overlayfs/copy_up.c | 2 +-
fs/overlayfs/export.c | 10 +++++-----
fs/overlayfs/inode.c | 8 ++++----
fs/overlayfs/namei.c | 2 +-
fs/overlayfs/ovl_entry.h | 16 ++++++++++++++++
fs/overlayfs/super.c | 12 ++++++------
fs/overlayfs/util.c | 18 +++++++++---------
7 files changed, 42 insertions(+), 26 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f658cc8ea492..60aa615820e7 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -905,7 +905,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
int flags)
{
- struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
if (!ofs->config.metacopy)
return false;
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index defd4e231ad2..f5f0ef8e3ce8 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -182,7 +182,7 @@ static int ovl_connect_layer(struct dentry *dentry)
*/
static int ovl_check_encode_origin(struct dentry *dentry)
{
- struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
/* Upper file handle for pure upper */
if (!ovl_dentry_lower(dentry))
@@ -434,7 +434,7 @@ static struct dentry *ovl_lookup_real_inode(struct super_block *sb,
struct dentry *real,
const struct ovl_layer *layer)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
struct dentry *index = NULL;
struct dentry *this = NULL;
struct inode *inode;
@@ -655,7 +655,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
struct ovl_path *lowerpath,
struct dentry *index)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
const struct ovl_layer *layer = upper ? &ofs->layers[0] : lowerpath->layer;
struct dentry *real = upper ?: (index ?: lowerpath->dentry);
@@ -680,7 +680,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
struct ovl_fh *fh)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
struct dentry *dentry;
struct dentry *upper;
@@ -700,7 +700,7 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
struct ovl_fh *fh)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
struct ovl_path origin = { };
struct ovl_path *stack = &origin;
struct dentry *dentry = NULL;
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 541cf3717fc2..c27823f6e7aa 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -334,7 +334,7 @@ static const char *ovl_get_link(struct dentry *dentry,
bool ovl_is_private_xattr(struct super_block *sb, const char *name)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
if (ofs->config.userxattr)
return strncmp(name, OVL_XATTR_USER_PREFIX,
@@ -689,7 +689,7 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
{
if (flags & S_ATIME) {
- struct ovl_fs *ofs = inode->i_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(inode->i_sb);
struct path upperpath = {
.mnt = ovl_upper_mnt(ofs),
.dentry = ovl_upperdentry_dereference(OVL_I(inode)),
@@ -952,7 +952,7 @@ static inline void ovl_lockdep_annotate_inode_mutex_key(struct inode *inode)
static void ovl_next_ino(struct inode *inode)
{
- struct ovl_fs *ofs = inode->i_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(inode->i_sb);
inode->i_ino = atomic_long_inc_return(&ofs->last_ino);
if (unlikely(!inode->i_ino))
@@ -1284,7 +1284,7 @@ struct inode *ovl_get_trap_inode(struct super_block *sb, struct dentry *dir)
static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
struct dentry *lower, bool index)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
/* No, if pure upper */
if (!lower)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index cfb3420b7df0..d0f196b85541 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -832,7 +832,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
{
struct ovl_entry *oe;
const struct cred *old_cred;
- struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
struct ovl_entry *poe = dentry->d_parent->d_fsdata;
struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
struct ovl_path *stack = NULL, *origin_path = NULL;
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index fd11fe6d6d45..0b93b1d9ad79 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -95,8 +95,24 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
return mnt_idmap(ovl_upper_mnt(ofs));
}
+extern struct file_system_type ovl_fs_type;
+
+#ifdef CONFIG_OVERLAY_FS_DEBUG
+static inline bool is_ovl_fs_sb(struct super_block *sb)
+{
+ return sb->s_type == &ovl_fs_type;
+}
+#else
+static inline bool is_ovl_fs_sb(struct super_block *sb)
+{
+ return true;
+}
+#endif
+
static inline struct ovl_fs *OVL_FS(struct super_block *sb)
{
+ WARN_ON_ONCE(!is_ovl_fs_sb(sb));
+
return (struct ovl_fs *)sb->s_fs_info;
}
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f97ad8b40dbb..2ed0f498fce4 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -259,7 +259,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
static void ovl_put_super(struct super_block *sb)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
ovl_free_fs(ofs);
}
@@ -267,7 +267,7 @@ static void ovl_put_super(struct super_block *sb)
/* Sync real dirty inodes in upper filesystem (if it exists) */
static int ovl_sync_fs(struct super_block *sb, int wait)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
struct super_block *upper_sb;
int ret;
@@ -315,7 +315,7 @@ static int ovl_sync_fs(struct super_block *sb, int wait)
*/
static int ovl_statfs(struct dentry *dentry, struct kstatfs *buf)
{
- struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
struct dentry *root_dentry = dentry->d_sb->s_root;
struct path path;
int err;
@@ -364,7 +364,7 @@ static inline int ovl_xino_def(void)
static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
{
struct super_block *sb = dentry->d_sb;
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
seq_show_option(m, "lowerdir", ofs->config.lowerdir);
if (ofs->config.upperdir) {
@@ -396,7 +396,7 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
static int ovl_remount(struct super_block *sb, int *flags, char *data)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
struct super_block *upper_sb;
int ret = 0;
@@ -2083,7 +2083,7 @@ static struct dentry *ovl_mount(struct file_system_type *fs_type, int flags,
return mount_nodev(fs_type, flags, raw_data, ovl_fill_super);
}
-static struct file_system_type ovl_fs_type = {
+struct file_system_type ovl_fs_type = {
.owner = THIS_MODULE,
.name = "overlay",
.fs_flags = FS_USERNS_MOUNT,
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 923d66d131c1..2425240ef139 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -18,25 +18,25 @@
int ovl_want_write(struct dentry *dentry)
{
- struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
return mnt_want_write(ovl_upper_mnt(ofs));
}
void ovl_drop_write(struct dentry *dentry)
{
- struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
mnt_drop_write(ovl_upper_mnt(ofs));
}
struct dentry *ovl_workdir(struct dentry *dentry)
{
- struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
return ofs->workdir;
}
const struct cred *ovl_override_creds(struct super_block *sb)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
return override_creds(ofs->creator_cred);
}
@@ -62,7 +62,7 @@ int ovl_can_decode_fh(struct super_block *sb)
struct dentry *ovl_indexdir(struct super_block *sb)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
return ofs->indexdir;
}
@@ -70,7 +70,7 @@ struct dentry *ovl_indexdir(struct super_block *sb)
/* Index all files on copy up. For now only enabled for NFS export */
bool ovl_index_all(struct super_block *sb)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
return ofs->config.nfs_export && ofs->config.index;
}
@@ -78,7 +78,7 @@ bool ovl_index_all(struct super_block *sb)
/* Verify lower origin on lookup. For now only enabled for NFS export */
bool ovl_verify_lower(struct super_block *sb)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
return ofs->config.nfs_export && ofs->config.index;
}
@@ -152,7 +152,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
void ovl_path_upper(struct dentry *dentry, struct path *path)
{
- struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
path->mnt = ovl_upper_mnt(ofs);
path->dentry = ovl_dentry_upper(dentry);
@@ -415,7 +415,7 @@ bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags)
bool ovl_redirect_dir(struct super_block *sb)
{
- struct ovl_fs *ofs = sb->s_fs_info;
+ struct ovl_fs *ofs = OVL_FS(sb);
return ofs->config.redirect_dir && !ofs->noxattr;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG
2023-05-20 18:41 ` [PATCH v2 1/2] ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG Andrea Righi
@ 2023-05-21 6:36 ` Amir Goldstein
0 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2023-05-21 6:36 UTC (permalink / raw)
To: Andrea Righi; +Cc: Miklos Szeredi, linux-unionfs, linux-kernel
On Sat, May 20, 2023 at 9:41 PM Andrea Righi <andrea.righi@canonical.com> wrote:
>
> Provide a Kconfig option to enable extra debugging checks for overlayfs.
>
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
I think that could be useful someday...
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/Kconfig | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index 6708e54b0e30..fec5020c3495 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -124,3 +124,12 @@ config OVERLAY_FS_METACOPY
> that doesn't support this feature will have unexpected results.
>
> If unsure, say N.
> +
> +config OVERLAY_FS_DEBUG
> + bool "Overlayfs: turn on extra debugging checks"
> + default n
> + depends on OVERLAY_FS
> + help
> + Say Y here to enable extra debugging checks in overlayfs.
> +
> + If unsure, say N.
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] ovl: make consistent use of OVL_FS()
2023-05-20 18:41 ` [PATCH v2 2/2] ovl: make consistent use of OVL_FS() Andrea Righi
@ 2023-05-21 6:51 ` Amir Goldstein
2023-05-21 8:08 ` Andrea Righi
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2023-05-21 6:51 UTC (permalink / raw)
To: Andrea Righi; +Cc: Miklos Szeredi, linux-unionfs, linux-kernel
On Sat, May 20, 2023 at 9:41 PM Andrea Righi <andrea.righi@canonical.com> wrote:
>
> Always use OVL_FS() to retrieve the corresponding struct ovl_fs from a
> struct super_block.
>
> Moreover, make sure that it is exclusively used with an overlayfs
> superblock when CONFIG_OVERLAY_FS_DEBUG is enabled (otherwise trigger a
> WARN_ON_ONCE).
Seems that you do not mind learning how we usually do things...
so "Moreover", "Also" is usually an indication that this change needs to be
in a separate patch.
I think this is one of those cases.
>
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> ---
> fs/overlayfs/copy_up.c | 2 +-
> fs/overlayfs/export.c | 10 +++++-----
> fs/overlayfs/inode.c | 8 ++++----
> fs/overlayfs/namei.c | 2 +-
> fs/overlayfs/ovl_entry.h | 16 ++++++++++++++++
> fs/overlayfs/super.c | 12 ++++++------
> fs/overlayfs/util.c | 18 +++++++++---------
> 7 files changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index f658cc8ea492..60aa615820e7 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -905,7 +905,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
> static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
> int flags)
> {
> - struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
>
> if (!ofs->config.metacopy)
> return false;
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index defd4e231ad2..f5f0ef8e3ce8 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -182,7 +182,7 @@ static int ovl_connect_layer(struct dentry *dentry)
> */
> static int ovl_check_encode_origin(struct dentry *dentry)
> {
> - struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
>
> /* Upper file handle for pure upper */
> if (!ovl_dentry_lower(dentry))
> @@ -434,7 +434,7 @@ static struct dentry *ovl_lookup_real_inode(struct super_block *sb,
> struct dentry *real,
> const struct ovl_layer *layer)
> {
> - struct ovl_fs *ofs = sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(sb);
> struct dentry *index = NULL;
> struct dentry *this = NULL;
> struct inode *inode;
> @@ -655,7 +655,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
> struct ovl_path *lowerpath,
> struct dentry *index)
> {
> - struct ovl_fs *ofs = sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(sb);
> const struct ovl_layer *layer = upper ? &ofs->layers[0] : lowerpath->layer;
> struct dentry *real = upper ?: (index ?: lowerpath->dentry);
>
> @@ -680,7 +680,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
> static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
> struct ovl_fh *fh)
> {
> - struct ovl_fs *ofs = sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(sb);
> struct dentry *dentry;
> struct dentry *upper;
>
> @@ -700,7 +700,7 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
> static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
> struct ovl_fh *fh)
> {
> - struct ovl_fs *ofs = sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(sb);
> struct ovl_path origin = { };
> struct ovl_path *stack = &origin;
> struct dentry *dentry = NULL;
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 541cf3717fc2..c27823f6e7aa 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -334,7 +334,7 @@ static const char *ovl_get_link(struct dentry *dentry,
>
> bool ovl_is_private_xattr(struct super_block *sb, const char *name)
> {
> - struct ovl_fs *ofs = sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(sb);
>
> if (ofs->config.userxattr)
> return strncmp(name, OVL_XATTR_USER_PREFIX,
> @@ -689,7 +689,7 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
> int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
> {
> if (flags & S_ATIME) {
> - struct ovl_fs *ofs = inode->i_sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> struct path upperpath = {
> .mnt = ovl_upper_mnt(ofs),
> .dentry = ovl_upperdentry_dereference(OVL_I(inode)),
> @@ -952,7 +952,7 @@ static inline void ovl_lockdep_annotate_inode_mutex_key(struct inode *inode)
>
> static void ovl_next_ino(struct inode *inode)
> {
> - struct ovl_fs *ofs = inode->i_sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(inode->i_sb);
>
> inode->i_ino = atomic_long_inc_return(&ofs->last_ino);
> if (unlikely(!inode->i_ino))
> @@ -1284,7 +1284,7 @@ struct inode *ovl_get_trap_inode(struct super_block *sb, struct dentry *dir)
> static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
> struct dentry *lower, bool index)
> {
> - struct ovl_fs *ofs = sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(sb);
>
> /* No, if pure upper */
> if (!lower)
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index cfb3420b7df0..d0f196b85541 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -832,7 +832,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> {
> struct ovl_entry *oe;
> const struct cred *old_cred;
> - struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> struct ovl_entry *poe = dentry->d_parent->d_fsdata;
> struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
> struct ovl_path *stack = NULL, *origin_path = NULL;
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index fd11fe6d6d45..0b93b1d9ad79 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -95,8 +95,24 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> return mnt_idmap(ovl_upper_mnt(ofs));
> }
>
> +extern struct file_system_type ovl_fs_type;
> +
> +#ifdef CONFIG_OVERLAY_FS_DEBUG
> +static inline bool is_ovl_fs_sb(struct super_block *sb)
> +{
> + return sb->s_type == &ovl_fs_type;
> +}
> +#else
> +static inline bool is_ovl_fs_sb(struct super_block *sb)
> +{
> + return true;
> +}
> +#endif
> +
> static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> {
> + WARN_ON_ONCE(!is_ovl_fs_sb(sb));
> +
> return (struct ovl_fs *)sb->s_fs_info;
> }
>
IMO, is_ovl_fs_sb() is useful and no reason to hide it under
CONFIG_OVERLAY_FS_DEBUG.
IMO, only the fortification of OVL_FS() needs to be hidden inside
CONFIG_OVERLAY_FS_DEBUG.
With those minor comments fixed you may add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Thanks,
Amir.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] ovl: make consistent use of OVL_FS()
2023-05-21 6:51 ` Amir Goldstein
@ 2023-05-21 8:08 ` Andrea Righi
0 siblings, 0 replies; 6+ messages in thread
From: Andrea Righi @ 2023-05-21 8:08 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Miklos Szeredi, linux-unionfs, linux-kernel
On Sun, May 21, 2023 at 09:51:46AM +0300, Amir Goldstein wrote:
> On Sat, May 20, 2023 at 9:41 PM Andrea Righi <andrea.righi@canonical.com> wrote:
> >
> > Always use OVL_FS() to retrieve the corresponding struct ovl_fs from a
> > struct super_block.
> >
> > Moreover, make sure that it is exclusively used with an overlayfs
> > superblock when CONFIG_OVERLAY_FS_DEBUG is enabled (otherwise trigger a
> > WARN_ON_ONCE).
>
> Seems that you do not mind learning how we usually do things...
> so "Moreover", "Also" is usually an indication that this change needs to be
> in a separate patch.
> I think this is one of those cases.
OK, I'll split the "moreover" part in a separate patch. :)
>
> >
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > ---
> > fs/overlayfs/copy_up.c | 2 +-
> > fs/overlayfs/export.c | 10 +++++-----
> > fs/overlayfs/inode.c | 8 ++++----
> > fs/overlayfs/namei.c | 2 +-
> > fs/overlayfs/ovl_entry.h | 16 ++++++++++++++++
> > fs/overlayfs/super.c | 12 ++++++------
> > fs/overlayfs/util.c | 18 +++++++++---------
> > 7 files changed, 42 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index f658cc8ea492..60aa615820e7 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -905,7 +905,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
> > static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
> > int flags)
> > {
> > - struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> >
> > if (!ofs->config.metacopy)
> > return false;
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index defd4e231ad2..f5f0ef8e3ce8 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -182,7 +182,7 @@ static int ovl_connect_layer(struct dentry *dentry)
> > */
> > static int ovl_check_encode_origin(struct dentry *dentry)
> > {
> > - struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> >
> > /* Upper file handle for pure upper */
> > if (!ovl_dentry_lower(dentry))
> > @@ -434,7 +434,7 @@ static struct dentry *ovl_lookup_real_inode(struct super_block *sb,
> > struct dentry *real,
> > const struct ovl_layer *layer)
> > {
> > - struct ovl_fs *ofs = sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(sb);
> > struct dentry *index = NULL;
> > struct dentry *this = NULL;
> > struct inode *inode;
> > @@ -655,7 +655,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
> > struct ovl_path *lowerpath,
> > struct dentry *index)
> > {
> > - struct ovl_fs *ofs = sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(sb);
> > const struct ovl_layer *layer = upper ? &ofs->layers[0] : lowerpath->layer;
> > struct dentry *real = upper ?: (index ?: lowerpath->dentry);
> >
> > @@ -680,7 +680,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
> > static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
> > struct ovl_fh *fh)
> > {
> > - struct ovl_fs *ofs = sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(sb);
> > struct dentry *dentry;
> > struct dentry *upper;
> >
> > @@ -700,7 +700,7 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
> > static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
> > struct ovl_fh *fh)
> > {
> > - struct ovl_fs *ofs = sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(sb);
> > struct ovl_path origin = { };
> > struct ovl_path *stack = &origin;
> > struct dentry *dentry = NULL;
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 541cf3717fc2..c27823f6e7aa 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -334,7 +334,7 @@ static const char *ovl_get_link(struct dentry *dentry,
> >
> > bool ovl_is_private_xattr(struct super_block *sb, const char *name)
> > {
> > - struct ovl_fs *ofs = sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(sb);
> >
> > if (ofs->config.userxattr)
> > return strncmp(name, OVL_XATTR_USER_PREFIX,
> > @@ -689,7 +689,7 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
> > int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
> > {
> > if (flags & S_ATIME) {
> > - struct ovl_fs *ofs = inode->i_sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> > struct path upperpath = {
> > .mnt = ovl_upper_mnt(ofs),
> > .dentry = ovl_upperdentry_dereference(OVL_I(inode)),
> > @@ -952,7 +952,7 @@ static inline void ovl_lockdep_annotate_inode_mutex_key(struct inode *inode)
> >
> > static void ovl_next_ino(struct inode *inode)
> > {
> > - struct ovl_fs *ofs = inode->i_sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> >
> > inode->i_ino = atomic_long_inc_return(&ofs->last_ino);
> > if (unlikely(!inode->i_ino))
> > @@ -1284,7 +1284,7 @@ struct inode *ovl_get_trap_inode(struct super_block *sb, struct dentry *dir)
> > static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
> > struct dentry *lower, bool index)
> > {
> > - struct ovl_fs *ofs = sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(sb);
> >
> > /* No, if pure upper */
> > if (!lower)
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index cfb3420b7df0..d0f196b85541 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -832,7 +832,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> > {
> > struct ovl_entry *oe;
> > const struct cred *old_cred;
> > - struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > struct ovl_entry *poe = dentry->d_parent->d_fsdata;
> > struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
> > struct ovl_path *stack = NULL, *origin_path = NULL;
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index fd11fe6d6d45..0b93b1d9ad79 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -95,8 +95,24 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> > return mnt_idmap(ovl_upper_mnt(ofs));
> > }
> >
> > +extern struct file_system_type ovl_fs_type;
> > +
> > +#ifdef CONFIG_OVERLAY_FS_DEBUG
> > +static inline bool is_ovl_fs_sb(struct super_block *sb)
> > +{
> > + return sb->s_type == &ovl_fs_type;
> > +}
> > +#else
> > +static inline bool is_ovl_fs_sb(struct super_block *sb)
> > +{
> > + return true;
> > +}
> > +#endif
> > +
> > static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> > {
> > + WARN_ON_ONCE(!is_ovl_fs_sb(sb));
> > +
> > return (struct ovl_fs *)sb->s_fs_info;
> > }
> >
>
> IMO, is_ovl_fs_sb() is useful and no reason to hide it under
> CONFIG_OVERLAY_FS_DEBUG.
> IMO, only the fortification of OVL_FS() needs to be hidden inside
> CONFIG_OVERLAY_FS_DEBUG.
>
> With those minor comments fixed you may add:
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Makes sense. Will send a v3 with your changes.
Thanks for the review!
-Andrea
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-05-21 8:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-20 18:41 [PATCH v2 0/2] overlayfs: debugging check for valid superblock Andrea Righi
2023-05-20 18:41 ` [PATCH v2 1/2] ovl: Kconfig: introduce CONFIG_OVERLAY_FS_DEBUG Andrea Righi
2023-05-21 6:36 ` Amir Goldstein
2023-05-20 18:41 ` [PATCH v2 2/2] ovl: make consistent use of OVL_FS() Andrea Righi
2023-05-21 6:51 ` Amir Goldstein
2023-05-21 8:08 ` Andrea Righi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).