($INBOX_DIR/description missing)
 help / color / Atom feed
* [PATCH 0/3] Misc. redirect_dir=nofollow fixes
@ 2020-07-13 14:19 Amir Goldstein
  2020-07-13 14:19 ` [PATCH 1/3] ovl: force read-only sb on failure to create index dir Amir Goldstein
                   ` (3 more replies)
  0 siblings, 4 replies; 23+ messages in thread
From: Amir Goldstein @ 2020-07-13 14:19 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Vivek Goyal, linux-unionfs

Miklos, Vivek,

Following discussion on following an unsafe non-dir origin [1]
and in a addition to a fix for the reported null uuid case [2] and to
Vivek's doc clarification [3], I am proposing to piggy back existing
config redirect_dir=nofollow to also not follow non-dir origin.

Like in the case of non-dir origin, following redirects behavior was
added with no opt-out option in kernel v4.10.  Later security concerns
about following malformed redirects resulted in the redirect_dir=nofollow
config option.

Without giving too much thought into how unsafe it can be to follow
a bad origin, there is very low motication IMO to follow non-dir origin
with redirect_dir=nofollow, because it is a configuration that prefers
safety over correctness, so it just seems like the right thing to do.

The first two patches are independent bug fixes related to read-only
NFS export, which can be taken regardless of non-dir origin nofollow.
FYI, I found those bugs because I am using ro,index=off NFS export
configuration for the new overlay fsnotify snaphsot series.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/CAJfpegv9h7ubuGy_6K4OCdZd3R7Z4HGmCDB2L7mO5bVoGd6MSA@mail.gmail.com/
[2] https://lore.kernel.org/linux-unionfs/20200708131613.30038-1-amir73il@gmail.com/
[3] https://lore.kernel.org/linux-unionfs/20200709140220.GC150543@redhat.com/

Amir Goldstein (3):
  ovl: force read-only sb on failure to create index dir
  ovl: fix mount option checks for nfs_export with no upperdir
  ovl: do not follow non-dir origin with redirect_dir=nofollow

 Documentation/filesystems/overlayfs.rst |  4 +--
 fs/overlayfs/namei.c                    |  2 +-
 fs/overlayfs/super.c                    | 42 ++++++++++++++-----------
 3 files changed, 27 insertions(+), 21 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/3] ovl: force read-only sb on failure to create index dir
  2020-07-13 14:19 [PATCH 0/3] Misc. redirect_dir=nofollow fixes Amir Goldstein
@ 2020-07-13 14:19 ` Amir Goldstein
  2020-07-14 18:18   ` Vivek Goyal
  2020-07-15 20:03   ` Miklos Szeredi
  2020-07-13 14:19 ` [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir Amir Goldstein
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 23+ messages in thread
From: Amir Goldstein @ 2020-07-13 14:19 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Vivek Goyal, linux-unionfs

With index feature enabled, on failure to create index dir, overlay
is being mounted read-only.  However, we do not forbid user to remount
overlay read-write.  Fix that by setting ofs->workdir to NULL, which
prevents remount read-write.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 4b7cb2d98203..41d7fe2b8129 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1374,12 +1374,13 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
 		goto out;
 	}
 
+	/* index dir will act also as workdir */
+	iput(ofs->workdir_trap);
+	ofs->workdir_trap = NULL;
+	dput(ofs->workdir);
+	ofs->workdir = NULL;
 	ofs->indexdir = ovl_workdir_create(ofs, OVL_INDEXDIR_NAME, true);
 	if (ofs->indexdir) {
-		/* index dir will act also as workdir */
-		iput(ofs->workdir_trap);
-		ofs->workdir_trap = NULL;
-		dput(ofs->workdir);
 		ofs->workdir = dget(ofs->indexdir);
 
 		err = ovl_setup_trap(sb, ofs->indexdir, &ofs->indexdir_trap,
@@ -1884,7 +1885,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (!ovl_upper_mnt(ofs))
 		sb->s_flags |= SB_RDONLY;
 
-	if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
+	if (!ovl_force_readonly(ofs) && ofs->config.index) {
 		err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
 		if (err)
 			goto out_free_oe;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir
  2020-07-13 14:19 [PATCH 0/3] Misc. redirect_dir=nofollow fixes Amir Goldstein
  2020-07-13 14:19 ` [PATCH 1/3] ovl: force read-only sb on failure to create index dir Amir Goldstein
@ 2020-07-13 14:19 ` Amir Goldstein
  2020-07-14 14:52   ` Miklos Szeredi
  2020-07-15 20:05   ` Miklos Szeredi
  2020-07-13 14:19 ` [PATCH 3/3] ovl: do not follow non-dir origin with redirect_dir=nofollow Amir Goldstein
  2020-07-14 18:07 ` [PATCH 0/3] Misc. redirect_dir=nofollow fixes Vivek Goyal
  3 siblings, 2 replies; 23+ messages in thread
From: Amir Goldstein @ 2020-07-13 14:19 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Vivek Goyal, linux-unionfs

Without upperdir mount option, there is no index dir and the dependency
checks nfs_export => index for mount options parsing are incorrect.

Allow the combination nfs_export=on,index=off with no upperdir and move
the check for dependency redirect_dir=nofollow for non-upper mount case
to mount options parsing.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.rst |  4 ++--
 fs/overlayfs/super.c                    | 31 ++++++++++++++-----------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 660dbaf0b9b8..fcda5d6ba9ac 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -560,8 +560,8 @@ When the NFS export feature is enabled, all directory index entries are
 verified on mount time to check that upper file handles are not stale.
 This verification may cause significant overhead in some cases.
 
-Note: the mount options index=off,nfs_export=on are conflicting and will
-result in an error.
+Note: the mount options index=off,nfs_export=on are conflicting for a
+read-write mount and will result in an error.
 
 
 Testsuite
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 41d7fe2b8129..3c8b48a2766b 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -602,12 +602,19 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		}
 	}
 
-	/* Workdir is useless in non-upper mount */
-	if (!config->upperdir && config->workdir) {
-		pr_info("option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
-			config->workdir);
-		kfree(config->workdir);
-		config->workdir = NULL;
+	/* Workdir/index are useless in non-upper mount */
+	if (!config->upperdir) {
+		if (config->workdir) {
+			pr_info("option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
+				config->workdir);
+			kfree(config->workdir);
+			config->workdir = NULL;
+		}
+		if (config->index && index_opt) {
+			pr_info("option \"index=on\" is useless in a non-upper mount, ignore\n");
+			index_opt = false;
+		}
+		config->index = false;
 	}
 
 	err = ovl_parse_redirect_mode(config, config->redirect_mode);
@@ -644,11 +651,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 
 	/* Resolve nfs_export -> index dependency */
 	if (config->nfs_export && !config->index) {
-		if (nfs_export_opt && index_opt) {
+		if (!config->upperdir && config->redirect_follow) {
+			pr_info("NFS export requires \"redirect_dir=nofollow\" on non-upper mount, falling back to nfs_export=off.\n");
+			config->nfs_export = false;
+		} else if (nfs_export_opt && index_opt) {
 			pr_err("conflicting options: nfs_export=on,index=off\n");
 			return -EINVAL;
-		}
-		if (index_opt) {
+		} else if (index_opt) {
 			/*
 			 * There was an explicit index=off that resulted
 			 * in this conflict.
@@ -1616,10 +1625,6 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	if (!ofs->config.upperdir && numlower == 1) {
 		pr_err("at least 2 lowerdir are needed while upperdir nonexistent\n");
 		return ERR_PTR(-EINVAL);
-	} else if (!ofs->config.upperdir && ofs->config.nfs_export &&
-		   ofs->config.redirect_follow) {
-		pr_warn("NFS export requires \"redirect_dir=nofollow\" on non-upper mount, falling back to nfs_export=off.\n");
-		ofs->config.nfs_export = false;
 	}
 
 	stack = kcalloc(numlower, sizeof(struct path), GFP_KERNEL);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 3/3] ovl: do not follow non-dir origin with redirect_dir=nofollow
  2020-07-13 14:19 [PATCH 0/3] Misc. redirect_dir=nofollow fixes Amir Goldstein
  2020-07-13 14:19 ` [PATCH 1/3] ovl: force read-only sb on failure to create index dir Amir Goldstein
  2020-07-13 14:19 ` [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir Amir Goldstein
@ 2020-07-13 14:19 ` Amir Goldstein
  2020-07-14 18:07 ` [PATCH 0/3] Misc. redirect_dir=nofollow fixes Vivek Goyal
  3 siblings, 0 replies; 23+ messages in thread
From: Amir Goldstein @ 2020-07-13 14:19 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Vivek Goyal, linux-unionfs

Following non-dir origin can result in some bugs when underlying layers
are edited offline.  To be on the safe side, do not follow non-dir
origin when not following redirects.  This will make overlay lookup
with "redirect_dir=nofollow" behave as pre kernel v4.12 lookup, before
the introduction of the origin xattr.

Link: https://lore.kernel.org/linux-unionfs/CAJfpegv9h7ubuGy_6K4OCdZd3R7Z4HGmCDB2L7mO5bVoGd6MSA@mail.gmail.com/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index ae1c1216a038..31ee5a519736 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -861,7 +861,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			err = -EREMOTE;
 			goto out;
 		}
-		if (upperdentry && !d.is_dir) {
+		if (upperdentry && !d.is_dir && ofs->config.redirect_follow) {
 			unsigned int origin_ctr = 0;
 
 			/*
-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir
  2020-07-13 14:19 ` [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir Amir Goldstein
@ 2020-07-14 14:52   ` Miklos Szeredi
  2020-07-14 14:58     ` Amir Goldstein
  2020-07-15 20:05   ` Miklos Szeredi
  1 sibling, 1 reply; 23+ messages in thread
From: Miklos Szeredi @ 2020-07-14 14:52 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Vivek Goyal, overlayfs

On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Without upperdir mount option, there is no index dir and the dependency
> checks nfs_export => index for mount options parsing are incorrect.
>
> Allow the combination nfs_export=on,index=off with no upperdir and move
> the check for dependency redirect_dir=nofollow for non-upper mount case
> to mount options parsing.

Okay, but does this combination make any sense?

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir
  2020-07-14 14:52   ` Miklos Szeredi
@ 2020-07-14 14:58     ` Amir Goldstein
  2020-07-14 15:08       ` Miklos Szeredi
  0 siblings, 1 reply; 23+ messages in thread
From: Amir Goldstein @ 2020-07-14 14:58 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Vivek Goyal, overlayfs

On Tue, Jul 14, 2020 at 5:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Without upperdir mount option, there is no index dir and the dependency
> > checks nfs_export => index for mount options parsing are incorrect.
> >
> > Allow the combination nfs_export=on,index=off with no upperdir and move
> > the check for dependency redirect_dir=nofollow for non-upper mount case
> > to mount options parsing.
>
> Okay, but does this combination make any sense?

Do you mean configuration of non-upper exported to NFS?
Why not? Anyway, we allowed it and regressed it.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir
  2020-07-14 14:58     ` Amir Goldstein
@ 2020-07-14 15:08       ` Miklos Szeredi
  2020-07-14 15:20         ` Amir Goldstein
  0 siblings, 1 reply; 23+ messages in thread
From: Miklos Szeredi @ 2020-07-14 15:08 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Vivek Goyal, overlayfs

On Tue, Jul 14, 2020 at 4:58 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Jul 14, 2020 at 5:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Without upperdir mount option, there is no index dir and the dependency
> > > checks nfs_export => index for mount options parsing are incorrect.
> > >
> > > Allow the combination nfs_export=on,index=off with no upperdir and move
> > > the check for dependency redirect_dir=nofollow for non-upper mount case
> > > to mount options parsing.
> >
> > Okay, but does this combination make any sense?
>
> Do you mean configuration of non-upper exported to NFS?
> Why not? Anyway, we allowed it and regressed it.

Ah, right.  I was confused and thought we'd want to allow
nfs_eport=on,index=on with no upper dir.  But that's nonsense
obviously.

Thanks,
Miklos

>
> Thanks,
> Amir.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir
  2020-07-14 15:08       ` Miklos Szeredi
@ 2020-07-14 15:20         ` Amir Goldstein
  0 siblings, 0 replies; 23+ messages in thread
From: Amir Goldstein @ 2020-07-14 15:20 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Vivek Goyal, overlayfs

On Tue, Jul 14, 2020 at 6:09 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, Jul 14, 2020 at 4:58 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 5:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > Without upperdir mount option, there is no index dir and the dependency
> > > > checks nfs_export => index for mount options parsing are incorrect.
> > > >
> > > > Allow the combination nfs_export=on,index=off with no upperdir and move
> > > > the check for dependency redirect_dir=nofollow for non-upper mount case
> > > > to mount options parsing.
> > >
> > > Okay, but does this combination make any sense?
> >
> > Do you mean configuration of non-upper exported to NFS?
> > Why not? Anyway, we allowed it and regressed it.
>
> Ah, right.  I was confused and thought we'd want to allow
> nfs_eport=on,index=on with no upper dir.  But that's nonsense
> obviously.
>

Heh no.
FYI, I got to this because working on the configuration:
ro,upperdir=...,nfs_eport=on,index=off

for snapshot, since it's a (forced) read-only mount, even though it
has an upperdir and copy ups, it has no upper rename/unlink, so there is
no need for an index to follow from lower to upper nor to follow redirect
from upper to lower.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes
  2020-07-13 14:19 [PATCH 0/3] Misc. redirect_dir=nofollow fixes Amir Goldstein
                   ` (2 preceding siblings ...)
  2020-07-13 14:19 ` [PATCH 3/3] ovl: do not follow non-dir origin with redirect_dir=nofollow Amir Goldstein
@ 2020-07-14 18:07 ` Vivek Goyal
  2020-07-14 18:42   ` Amir Goldstein
  3 siblings, 1 reply; 23+ messages in thread
From: Vivek Goyal @ 2020-07-14 18:07 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, linux-unionfs

On Mon, Jul 13, 2020 at 05:19:42PM +0300, Amir Goldstein wrote:
> Miklos, Vivek,
> 
> Following discussion on following an unsafe non-dir origin [1]
> and in a addition to a fix for the reported null uuid case [2] and to
> Vivek's doc clarification [3], I am proposing to piggy back existing
> config redirect_dir=nofollow to also not follow non-dir origin.
> 
> Like in the case of non-dir origin, following redirects behavior was
> added with no opt-out option in kernel v4.10.  Later security concerns
> about following malformed redirects resulted in the redirect_dir=nofollow
> config option.

So what's the security issue you are seeing with malformed origin? If
it indeed is a security threat, then we should probably introduce
another mount option to disable it (instead of reusing redirect_dir,
because that's so unintuitive, IMHO).

Thanks
Vivek
> 
> Without giving too much thought into how unsafe it can be to follow
> a bad origin, there is very low motication IMO to follow non-dir origin
> with redirect_dir=nofollow, because it is a configuration that prefers
> safety over correctness, so it just seems like the right thing to do.
> 
> The first two patches are independent bug fixes related to read-only
> NFS export, which can be taken regardless of non-dir origin nofollow.
> FYI, I found those bugs because I am using ro,index=off NFS export
> configuration for the new overlay fsnotify snaphsot series.
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-unionfs/CAJfpegv9h7ubuGy_6K4OCdZd3R7Z4HGmCDB2L7mO5bVoGd6MSA@mail.gmail.com/
> [2] https://lore.kernel.org/linux-unionfs/20200708131613.30038-1-amir73il@gmail.com/
> [3] https://lore.kernel.org/linux-unionfs/20200709140220.GC150543@redhat.com/
> 
> Amir Goldstein (3):
>   ovl: force read-only sb on failure to create index dir
>   ovl: fix mount option checks for nfs_export with no upperdir
>   ovl: do not follow non-dir origin with redirect_dir=nofollow
> 
>  Documentation/filesystems/overlayfs.rst |  4 +--
>  fs/overlayfs/namei.c                    |  2 +-
>  fs/overlayfs/super.c                    | 42 ++++++++++++++-----------
>  3 files changed, 27 insertions(+), 21 deletions(-)
> 
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir
  2020-07-13 14:19 ` [PATCH 1/3] ovl: force read-only sb on failure to create index dir Amir Goldstein
@ 2020-07-14 18:18   ` Vivek Goyal
  2020-07-14 18:32     ` Amir Goldstein
  2020-07-15 20:03   ` Miklos Szeredi
  1 sibling, 1 reply; 23+ messages in thread
From: Vivek Goyal @ 2020-07-14 18:18 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, linux-unionfs

On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> With index feature enabled, on failure to create index dir, overlay
> is being mounted read-only.  However, we do not forbid user to remount
> overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> prevents remount read-write.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

This patch does not apply for me. What branch you have generated it
against. I am using 5.8-rc4.

Vivek

> ---
>  fs/overlayfs/super.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 4b7cb2d98203..41d7fe2b8129 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1374,12 +1374,13 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
>  		goto out;
>  	}
>  
> +	/* index dir will act also as workdir */
> +	iput(ofs->workdir_trap);
> +	ofs->workdir_trap = NULL;
> +	dput(ofs->workdir);
> +	ofs->workdir = NULL;
>  	ofs->indexdir = ovl_workdir_create(ofs, OVL_INDEXDIR_NAME, true);
>  	if (ofs->indexdir) {
> -		/* index dir will act also as workdir */
> -		iput(ofs->workdir_trap);
> -		ofs->workdir_trap = NULL;
> -		dput(ofs->workdir);
>  		ofs->workdir = dget(ofs->indexdir);
>  
>  		err = ovl_setup_trap(sb, ofs->indexdir, &ofs->indexdir_trap,
> @@ -1884,7 +1885,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  	if (!ovl_upper_mnt(ofs))
>  		sb->s_flags |= SB_RDONLY;
>  
> -	if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
> +	if (!ovl_force_readonly(ofs) && ofs->config.index) {
>  		err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
>  		if (err)
>  			goto out_free_oe;
> -- 
> 2.17.1
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir
  2020-07-14 18:18   ` Vivek Goyal
@ 2020-07-14 18:32     ` Amir Goldstein
  2020-07-14 18:38       ` Vivek Goyal
  0 siblings, 1 reply; 23+ messages in thread
From: Amir Goldstein @ 2020-07-14 18:32 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: Miklos Szeredi, overlayfs

On Tue, Jul 14, 2020 at 9:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> > With index feature enabled, on failure to create index dir, overlay
> > is being mounted read-only.  However, we do not forbid user to remount
> > overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> > prevents remount read-write.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> This patch does not apply for me. What branch you have generated it
> against. I am using 5.8-rc4.

It's from my ovl-fixes branch.

Sorry I did not notice that it depends on a previous patch that Miklos
just picked up:

"ovl: fix oops in ovl_indexdir_cleanup() with nfs_export=on"

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir
  2020-07-14 18:32     ` Amir Goldstein
@ 2020-07-14 18:38       ` Vivek Goyal
  2020-07-14 18:45         ` Amir Goldstein
  2020-07-15 20:04         ` Miklos Szeredi
  0 siblings, 2 replies; 23+ messages in thread
From: Vivek Goyal @ 2020-07-14 18:38 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, overlayfs

On Tue, Jul 14, 2020 at 09:32:51PM +0300, Amir Goldstein wrote:
> On Tue, Jul 14, 2020 at 9:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> > > With index feature enabled, on failure to create index dir, overlay
> > > is being mounted read-only.  However, we do not forbid user to remount
> > > overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> > > prevents remount read-write.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > This patch does not apply for me. What branch you have generated it
> > against. I am using 5.8-rc4.
> 
> It's from my ovl-fixes branch.
> 
> Sorry I did not notice that it depends on a previous patch that Miklos
> just picked up:
> 
> "ovl: fix oops in ovl_indexdir_cleanup() with nfs_export=on"

I dont see it here.

https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next

Is there another tree/branch miklos is maintaining which I should use? Or
you just happen to know that Miklos has committed this internally and
not published yet.

Thanks
Vivek


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes
  2020-07-14 18:07 ` [PATCH 0/3] Misc. redirect_dir=nofollow fixes Vivek Goyal
@ 2020-07-14 18:42   ` Amir Goldstein
  2020-07-15 13:06     ` Vivek Goyal
  0 siblings, 1 reply; 23+ messages in thread
From: Amir Goldstein @ 2020-07-14 18:42 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: Miklos Szeredi, overlayfs

On Tue, Jul 14, 2020 at 9:07 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, Jul 13, 2020 at 05:19:42PM +0300, Amir Goldstein wrote:
> > Miklos, Vivek,
> >
> > Following discussion on following an unsafe non-dir origin [1]
> > and in a addition to a fix for the reported null uuid case [2] and to
> > Vivek's doc clarification [3], I am proposing to piggy back existing
> > config redirect_dir=nofollow to also not follow non-dir origin.
> >
> > Like in the case of non-dir origin, following redirects behavior was
> > added with no opt-out option in kernel v4.10.  Later security concerns
> > about following malformed redirects resulted in the redirect_dir=nofollow
> > config option.
>
> So what's the security issue you are seeing with malformed origin? If

TBH I never really understood the thread that led to redirect_dir=nofollow.
I don't think anyone has presented a proper use case that can be discussed,
so I just treat this config option as "paranoia" or "don't give me anything that
very old overlay did not give me".
Therefore I suggested piggybacking on it.
Of course if we do, we will need to document that.
BTW, I have another patch that does not follow mismatched lower dir origin
with redirect_dir=nofollow, i.e.:

 bool ovl_verify_lower(struct super_block *sb)
 {
        struct ovl_fs *ofs = sb->s_fs_info;

-       return ofs->config.nfs_export && ofs->config.index;
+       return ofs->config.nfs_export || !ofs->config.redirect_follow;
 }

That makes even more sense for "paranoia" IMO.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir
  2020-07-14 18:38       ` Vivek Goyal
@ 2020-07-14 18:45         ` Amir Goldstein
  2020-07-15 20:04         ` Miklos Szeredi
  1 sibling, 0 replies; 23+ messages in thread
From: Amir Goldstein @ 2020-07-14 18:45 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: Miklos Szeredi, overlayfs

On Tue, Jul 14, 2020 at 9:38 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Jul 14, 2020 at 09:32:51PM +0300, Amir Goldstein wrote:
> > On Tue, Jul 14, 2020 at 9:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> > > > With index feature enabled, on failure to create index dir, overlay
> > > > is being mounted read-only.  However, we do not forbid user to remount
> > > > overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> > > > prevents remount read-write.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > This patch does not apply for me. What branch you have generated it
> > > against. I am using 5.8-rc4.
> >
> > It's from my ovl-fixes branch.
> >
> > Sorry I did not notice that it depends on a previous patch that Miklos
> > just picked up:
> >
> > "ovl: fix oops in ovl_indexdir_cleanup() with nfs_export=on"
>
> I dont see it here.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next
>
> Is there another tree/branch miklos is maintaining which I should use? Or
> you just happen to know that Miklos has committed this internally and
> not published yet.
>

Sorry. He just replied to the email "applied" a few hours ago.
If you need the patch it's on my ovl-fixes branch in github.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes
  2020-07-14 18:42   ` Amir Goldstein
@ 2020-07-15 13:06     ` Vivek Goyal
  2020-07-15 13:56       ` Amir Goldstein
  0 siblings, 1 reply; 23+ messages in thread
From: Vivek Goyal @ 2020-07-15 13:06 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, overlayfs

On Tue, Jul 14, 2020 at 09:42:53PM +0300, Amir Goldstein wrote:
> On Tue, Jul 14, 2020 at 9:07 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 05:19:42PM +0300, Amir Goldstein wrote:
> > > Miklos, Vivek,
> > >
> > > Following discussion on following an unsafe non-dir origin [1]
> > > and in a addition to a fix for the reported null uuid case [2] and to
> > > Vivek's doc clarification [3], I am proposing to piggy back existing
> > > config redirect_dir=nofollow to also not follow non-dir origin.
> > >
> > > Like in the case of non-dir origin, following redirects behavior was
> > > added with no opt-out option in kernel v4.10.  Later security concerns
> > > about following malformed redirects resulted in the redirect_dir=nofollow
> > > config option.
> >
> > So what's the security issue you are seeing with malformed origin? If
> 
> TBH I never really understood the thread that led to redirect_dir=nofollow.
> I don't think anyone has presented a proper use case that can be discussed,

IIUC, idea was that automated mounting can mount a handcrafted upper on
usb hence allow access to directories on host which are otherwise
inaccessible. 

> so I just treat this config option as "paranoia" or "don't give me anything that
> very old overlay did not give me".
> Therefore I suggested piggybacking on it.

Even if it is paranoia, put more unrelated checks under this option does
not make much sense to me. It will make things just more confusing.

Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
not follow origin, then we first need to have a genuine explanation
of why to do that (and not be driven by just paranoia).

> Of course if we do, we will need to document that.

redirect_dir=nofollow resulting in origin not being followed is plain
unintuitive to me. Why not introduce another option if not following
origin is so important.

Thanks
Vivek

> BTW, I have another patch that does not follow mismatched lower dir origin
> with redirect_dir=nofollow, i.e.:
> 
>  bool ovl_verify_lower(struct super_block *sb)
>  {
>         struct ovl_fs *ofs = sb->s_fs_info;
> 
> -       return ofs->config.nfs_export && ofs->config.index;
> +       return ofs->config.nfs_export || !ofs->config.redirect_follow;
>  }
> 
> That makes even more sense for "paranoia" IMO.
> 
> Thanks,
> Amir.
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes
  2020-07-15 13:06     ` Vivek Goyal
@ 2020-07-15 13:56       ` Amir Goldstein
  2020-07-16 13:27         ` Vivek Goyal
  0 siblings, 1 reply; 23+ messages in thread
From: Amir Goldstein @ 2020-07-15 13:56 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: Miklos Szeredi, overlayfs

> > TBH I never really understood the thread that led to redirect_dir=nofollow.
> > I don't think anyone has presented a proper use case that can be discussed,
>
> IIUC, idea was that automated mounting can mount a handcrafted upper on
> usb hence allow access to directories on host which are otherwise
> inaccessible.
>

That is an *idea* described by hand waving.
That is not a threat I can seriously comment on.
How exactly is that USB auto mounted? where to?
How is that related to overlay?

> > so I just treat this config option as "paranoia" or "don't give me anything that
> > very old overlay did not give me".
> > Therefore I suggested piggybacking on it.
>
> Even if it is paranoia, put more unrelated checks under this option does
> not make much sense to me. It will make things just more confusing.
>
> Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
> not follow origin, then we first need to have a genuine explanation
> of why to do that (and not be driven by just paranoia).
>
> > Of course if we do, we will need to document that.
>
> redirect_dir=nofollow resulting in origin not being followed is plain
> unintuitive to me. Why not introduce another option if not following
> origin is so important.
>

Because cluttering the user with more and more config options for
minor and mostly unimportant behaviors is not ideal either.
See what Kconfig help has to say about the config option:

config OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW
        bool "Overlayfs: follow redirects even if redirects are turned off"
        default y

       Disable this to get a possibly more secure configuration, but that
       might not be backward compatible with previous kernels.

That is a VERY generic description that fits not following origin very
well IMO, and not following unverified dir origin as well for that matter.
Nobody outside overlayfs developers knows what "redirects" means
anyway. To me, following non-dir origin sounds exactly the same
as following non-dir metacopy redirect or dir redirect. It's just the
implementation details that differ.

So my claim is that we *can* piggyback on it because I really
don't believe anybody is using this config out there for anything
other than "to be on the safe side".

But I do not make the calls here and it doesn't look like I managed
to convince you to take my side of the argument :-)

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir
  2020-07-13 14:19 ` [PATCH 1/3] ovl: force read-only sb on failure to create index dir Amir Goldstein
  2020-07-14 18:18   ` Vivek Goyal
@ 2020-07-15 20:03   ` Miklos Szeredi
  1 sibling, 0 replies; 23+ messages in thread
From: Miklos Szeredi @ 2020-07-15 20:03 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Vivek Goyal, overlayfs

On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> With index feature enabled, on failure to create index dir, overlay
> is being mounted read-only.  However, we do not forbid user to remount
> overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> prevents remount read-write.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Applied, thanks.

Miklos

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir
  2020-07-14 18:38       ` Vivek Goyal
  2020-07-14 18:45         ` Amir Goldstein
@ 2020-07-15 20:04         ` Miklos Szeredi
  2020-07-16  5:00           ` Amir Goldstein
  1 sibling, 1 reply; 23+ messages in thread
From: Miklos Szeredi @ 2020-07-15 20:04 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: Amir Goldstein, overlayfs

On Tue, Jul 14, 2020 at 8:38 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Jul 14, 2020 at 09:32:51PM +0300, Amir Goldstein wrote:
> > On Tue, Jul 14, 2020 at 9:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Mon, Jul 13, 2020 at 05:19:43PM +0300, Amir Goldstein wrote:
> > > > With index feature enabled, on failure to create index dir, overlay
> > > > is being mounted read-only.  However, we do not forbid user to remount
> > > > overlay read-write.  Fix that by setting ofs->workdir to NULL, which
> > > > prevents remount read-write.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > This patch does not apply for me. What branch you have generated it
> > > against. I am using 5.8-rc4.
> >
> > It's from my ovl-fixes branch.
> >
> > Sorry I did not notice that it depends on a previous patch that Miklos
> > just picked up:
> >
> > "ovl: fix oops in ovl_indexdir_cleanup() with nfs_export=on"
>
> I dont see it here.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next
>
> Is there another tree/branch miklos is maintaining which I should use? Or
> you just happen to know that Miklos has committed this internally and
> not published yet.

Will push shortly to #overlayfs-next.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir
  2020-07-13 14:19 ` [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir Amir Goldstein
  2020-07-14 14:52   ` Miklos Szeredi
@ 2020-07-15 20:05   ` Miklos Szeredi
  1 sibling, 0 replies; 23+ messages in thread
From: Miklos Szeredi @ 2020-07-15 20:05 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Vivek Goyal, overlayfs

On Mon, Jul 13, 2020 at 4:19 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Without upperdir mount option, there is no index dir and the dependency
> checks nfs_export => index for mount options parsing are incorrect.
>
> Allow the combination nfs_export=on,index=off with no upperdir and move
> the check for dependency redirect_dir=nofollow for non-upper mount case
> to mount options parsing.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Applied, thanks.

Miklos

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/3] ovl: force read-only sb on failure to create index dir
  2020-07-15 20:04         ` Miklos Szeredi
@ 2020-07-16  5:00           ` Amir Goldstein
  0 siblings, 0 replies; 23+ messages in thread
From: Amir Goldstein @ 2020-07-16  5:00 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Vivek Goyal, overlayfs

> > I dont see it here.
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next
> >
> > Is there another tree/branch miklos is maintaining which I should use? Or
> > you just happen to know that Miklos has committed this internally and
> > not published yet.
>
> Will push shortly to #overlayfs-next.
>

Miklos,

Thanks for the notice.
I think it is good practice to announce when #overlayfs-next is updated and
ask if anyone knows of a patch that fell under the chairs.
I see that other maintainers do that sometimes.

I am missing two of my patches that I wonder if you left out intentionally
or did not get to yet:

1. ovl: fix unneeded call to ovl_change_flags()

One month old posting, so maybe it skipped your radar.
Seems like an obvious fix and needed for stable too.

2. ovl: fix lookup of indexed hardlinks with metacopy

From yesterday, so perhaps you did not get to it yet.
v5.8-rc1 regression.

They are both pushed to my rebased ovl-fixes branch.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes
  2020-07-15 13:56       ` Amir Goldstein
@ 2020-07-16 13:27         ` Vivek Goyal
  2020-07-16 13:43           ` Amir Goldstein
  0 siblings, 1 reply; 23+ messages in thread
From: Vivek Goyal @ 2020-07-16 13:27 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, overlayfs

On Wed, Jul 15, 2020 at 04:56:45PM +0300, Amir Goldstein wrote:
> > > TBH I never really understood the thread that led to redirect_dir=nofollow.
> > > I don't think anyone has presented a proper use case that can be discussed,
> >
> > IIUC, idea was that automated mounting can mount a handcrafted upper on
> > usb hence allow access to directories on host which are otherwise
> > inaccessible.
> >
> 
> That is an *idea* described by hand waving.
> That is not a threat I can seriously comment on.
> How exactly is that USB auto mounted? where to?
> How is that related to overlay?
> 
> > > so I just treat this config option as "paranoia" or "don't give me anything that
> > > very old overlay did not give me".
> > > Therefore I suggested piggybacking on it.
> >
> > Even if it is paranoia, put more unrelated checks under this option does
> > not make much sense to me. It will make things just more confusing.
> >
> > Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
> > not follow origin, then we first need to have a genuine explanation
> > of why to do that (and not be driven by just paranoia).
> >
> > > Of course if we do, we will need to document that.
> >
> > redirect_dir=nofollow resulting in origin not being followed is plain
> > unintuitive to me. Why not introduce another option if not following
> > origin is so important.
> >
> 
> Because cluttering the user with more and more config options for
> minor and mostly unimportant behaviors is not ideal either.
> See what Kconfig help has to say about the config option:
> 
> config OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW
>         bool "Overlayfs: follow redirects even if redirects are turned off"
>         default y
> 
>        Disable this to get a possibly more secure configuration, but that
>        might not be backward compatible with previous kernels.
> 
> That is a VERY generic description that fits not following origin very
> well IMO, and not following unverified dir origin as well for that matter.
> Nobody outside overlayfs developers knows what "redirects" means
> anyway. To me, following non-dir origin sounds exactly the same
> as following non-dir metacopy redirect or dir redirect. It's just the
> implementation details that differ.
> 
> So my claim is that we *can* piggyback on it because I really
> don't believe anybody is using this config out there for anything
> other than "to be on the safe side".

On one hand you are saying redirect=nofollow is paranoia, most people
don't understand it and don't use it. And on top of that you want
to add more to it. Adding more to something which nobody does not
understand and uses, sounds like more trouble to me.

Anyway, before we go further into this, what's the use case. Why
do you want to provide option to disable following origin for non-dir?

Thanks
Vivek

> 
> But I do not make the calls here and it doesn't look like I managed
> to convince you to take my side of the argument :-)
> 
> Thanks,
> Amir.
> 


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes
  2020-07-16 13:27         ` Vivek Goyal
@ 2020-07-16 13:43           ` Amir Goldstein
  2020-07-16 15:26             ` Vivek Goyal
  0 siblings, 1 reply; 23+ messages in thread
From: Amir Goldstein @ 2020-07-16 13:43 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: Miklos Szeredi, overlayfs

On Thu, Jul 16, 2020 at 4:27 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Jul 15, 2020 at 04:56:45PM +0300, Amir Goldstein wrote:
> > > > TBH I never really understood the thread that led to redirect_dir=nofollow.
> > > > I don't think anyone has presented a proper use case that can be discussed,
> > >
> > > IIUC, idea was that automated mounting can mount a handcrafted upper on
> > > usb hence allow access to directories on host which are otherwise
> > > inaccessible.
> > >
> >
> > That is an *idea* described by hand waving.
> > That is not a threat I can seriously comment on.
> > How exactly is that USB auto mounted? where to?
> > How is that related to overlay?
> >
> > > > so I just treat this config option as "paranoia" or "don't give me anything that
> > > > very old overlay did not give me".
> > > > Therefore I suggested piggybacking on it.
> > >
> > > Even if it is paranoia, put more unrelated checks under this option does
> > > not make much sense to me. It will make things just more confusing.
> > >
> > > Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
> > > not follow origin, then we first need to have a genuine explanation
> > > of why to do that (and not be driven by just paranoia).
> > >
> > > > Of course if we do, we will need to document that.
> > >
> > > redirect_dir=nofollow resulting in origin not being followed is plain
> > > unintuitive to me. Why not introduce another option if not following
> > > origin is so important.
> > >
> >
> > Because cluttering the user with more and more config options for
> > minor and mostly unimportant behaviors is not ideal either.
> > See what Kconfig help has to say about the config option:
> >
> > config OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW
> >         bool "Overlayfs: follow redirects even if redirects are turned off"
> >         default y
> >
> >        Disable this to get a possibly more secure configuration, but that
> >        might not be backward compatible with previous kernels.
> >
> > That is a VERY generic description that fits not following origin very
> > well IMO, and not following unverified dir origin as well for that matter.
> > Nobody outside overlayfs developers knows what "redirects" means
> > anyway. To me, following non-dir origin sounds exactly the same
> > as following non-dir metacopy redirect or dir redirect. It's just the
> > implementation details that differ.
> >
> > So my claim is that we *can* piggyback on it because I really
> > don't believe anybody is using this config out there for anything
> > other than "to be on the safe side".
>
> On one hand you are saying redirect=nofollow is paranoia, most people
> don't understand it and don't use it. And on top of that you want
> to add more to it. Adding more to something which nobody does not
> understand and uses, sounds like more trouble to me.
>

I am sorry, my POV is exactly the opposite of that.
No need to argue about it though ;-)

> Anyway, before we go further into this, what's the use case. Why
> do you want to provide option to disable following origin for non-dir?
>

I started thinking about this because of the squashfs bug report
(replacing lower layers) for which I had sent a patch to automatically
disable non-dir origin.

Reproducing the same problems with underlying fs with uuid is harder
but not impossible to think of a scenario.
My line of thinking is why should force feed the users with a feature they
didn't ask for if it can break something.
The very least we could do is allow users to opt-out.

But then again, if no one complains, we don't really need to do anything.
I have that feature (no follow origin) anyway for snapshot, but I can enable
it only for the snapshot case, so I don't mind if it can be enabled with
another config option or not - just wanted to put it out there for discussion.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Misc. redirect_dir=nofollow fixes
  2020-07-16 13:43           ` Amir Goldstein
@ 2020-07-16 15:26             ` Vivek Goyal
  0 siblings, 0 replies; 23+ messages in thread
From: Vivek Goyal @ 2020-07-16 15:26 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Miklos Szeredi, overlayfs

On Thu, Jul 16, 2020 at 04:43:22PM +0300, Amir Goldstein wrote:
> On Thu, Jul 16, 2020 at 4:27 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Jul 15, 2020 at 04:56:45PM +0300, Amir Goldstein wrote:
> > > > > TBH I never really understood the thread that led to redirect_dir=nofollow.
> > > > > I don't think anyone has presented a proper use case that can be discussed,
> > > >
> > > > IIUC, idea was that automated mounting can mount a handcrafted upper on
> > > > usb hence allow access to directories on host which are otherwise
> > > > inaccessible.
> > > >
> > >
> > > That is an *idea* described by hand waving.
> > > That is not a threat I can seriously comment on.
> > > How exactly is that USB auto mounted? where to?
> > > How is that related to overlay?
> > >
> > > > > so I just treat this config option as "paranoia" or "don't give me anything that
> > > > > very old overlay did not give me".
> > > > > Therefore I suggested piggybacking on it.
> > > >
> > > > Even if it is paranoia, put more unrelated checks under this option does
> > > > not make much sense to me. It will make things just more confusing.
> > > >
> > > > Anyway, redirect_dir=nofollow is a thing of past. Now if you want to
> > > > not follow origin, then we first need to have a genuine explanation
> > > > of why to do that (and not be driven by just paranoia).
> > > >
> > > > > Of course if we do, we will need to document that.
> > > >
> > > > redirect_dir=nofollow resulting in origin not being followed is plain
> > > > unintuitive to me. Why not introduce another option if not following
> > > > origin is so important.
> > > >
> > >
> > > Because cluttering the user with more and more config options for
> > > minor and mostly unimportant behaviors is not ideal either.
> > > See what Kconfig help has to say about the config option:
> > >
> > > config OVERLAY_FS_REDIRECT_ALWAYS_FOLLOW
> > >         bool "Overlayfs: follow redirects even if redirects are turned off"
> > >         default y
> > >
> > >        Disable this to get a possibly more secure configuration, but that
> > >        might not be backward compatible with previous kernels.
> > >
> > > That is a VERY generic description that fits not following origin very
> > > well IMO, and not following unverified dir origin as well for that matter.
> > > Nobody outside overlayfs developers knows what "redirects" means
> > > anyway. To me, following non-dir origin sounds exactly the same
> > > as following non-dir metacopy redirect or dir redirect. It's just the
> > > implementation details that differ.
> > >
> > > So my claim is that we *can* piggyback on it because I really
> > > don't believe anybody is using this config out there for anything
> > > other than "to be on the safe side".
> >
> > On one hand you are saying redirect=nofollow is paranoia, most people
> > don't understand it and don't use it. And on top of that you want
> > to add more to it. Adding more to something which nobody does not
> > understand and uses, sounds like more trouble to me.
> >
> 
> I am sorry, my POV is exactly the opposite of that.
> No need to argue about it though ;-)
> 
> > Anyway, before we go further into this, what's the use case. Why
> > do you want to provide option to disable following origin for non-dir?
> >
> 
> I started thinking about this because of the squashfs bug report
> (replacing lower layers) for which I had sent a patch to automatically
> disable non-dir origin.
> 
> Reproducing the same problems with underlying fs with uuid is harder
> but not impossible to think of a scenario.
> My line of thinking is why should force feed the users with a feature they
> didn't ask for if it can break something.
> The very least we could do is allow users to opt-out.
> 
> But then again, if no one complains, we don't really need to do anything.
> I have that feature (no follow origin) anyway for snapshot, but I can enable
> it only for the snapshot case, so I don't mind if it can be enabled with
> another config option or not - just wanted to put it out there for discussion.

"origin" seems to be more of an internal detail of overlayfs at this
point of time. So I agree that instead of providing another
opetion to disable it now, we can wait if somebody really runs into
issues. And you can disable it for snapshofts internally if you really
have to.

Thanks
Vivek


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-13 14:19 [PATCH 0/3] Misc. redirect_dir=nofollow fixes Amir Goldstein
2020-07-13 14:19 ` [PATCH 1/3] ovl: force read-only sb on failure to create index dir Amir Goldstein
2020-07-14 18:18   ` Vivek Goyal
2020-07-14 18:32     ` Amir Goldstein
2020-07-14 18:38       ` Vivek Goyal
2020-07-14 18:45         ` Amir Goldstein
2020-07-15 20:04         ` Miklos Szeredi
2020-07-16  5:00           ` Amir Goldstein
2020-07-15 20:03   ` Miklos Szeredi
2020-07-13 14:19 ` [PATCH 2/3] ovl: fix mount option checks for nfs_export with no upperdir Amir Goldstein
2020-07-14 14:52   ` Miklos Szeredi
2020-07-14 14:58     ` Amir Goldstein
2020-07-14 15:08       ` Miklos Szeredi
2020-07-14 15:20         ` Amir Goldstein
2020-07-15 20:05   ` Miklos Szeredi
2020-07-13 14:19 ` [PATCH 3/3] ovl: do not follow non-dir origin with redirect_dir=nofollow Amir Goldstein
2020-07-14 18:07 ` [PATCH 0/3] Misc. redirect_dir=nofollow fixes Vivek Goyal
2020-07-14 18:42   ` Amir Goldstein
2020-07-15 13:06     ` Vivek Goyal
2020-07-15 13:56       ` Amir Goldstein
2020-07-16 13:27         ` Vivek Goyal
2020-07-16 13:43           ` Amir Goldstein
2020-07-16 15:26             ` Vivek Goyal

($INBOX_DIR/description missing)

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-unionfs/0 linux-unionfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-unionfs linux-unionfs/ https://lore.kernel.org/linux-unionfs \
		linux-unionfs@vger.kernel.org
	public-inbox-index linux-unionfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-unionfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git