* [PATCH][V2] ovl: fix lookup failure on multi lower squashfs
@ 2019-11-07 10:49 Colin King
2019-11-12 9:02 ` Colin Ian King
2019-11-13 16:02 ` Miklos Szeredi
0 siblings, 2 replies; 7+ messages in thread
From: Colin King @ 2019-11-07 10:49 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein, linux-unionfs, stable
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
In the past, overlayfs required that lower fs have non null uuid in
order to support nfs export and decode copy up origin file handles.
Commit 9df085f3c9a2 ("ovl: relax requirement for non null uuid of
lower fs") relaxed this requirement for nfs export support, as long
as uuid (even if null) is unique among all lower fs.
However, said commit unintentionally also relaxed the non null uuid
requirement for decoding copy up origin file handles, regardless of
the unique uuid requirement.
Amend this mistake by disabling decoding of copy up origin file handle
from lower fs with a conflicting uuid.
We still encode copy up origin file handles from those fs, because
file handles like those already exist in the wild and because they
might provide useful information in the future.
Reported-by: Colin Ian King <colin.king@canonical.com>
Link: https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
Fixes: 9df085f3c9a2 ("ovl: relax requirement for non null uuid ...")
Cc: stable@vger.kernel.org # v4.20+
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
fs/overlayfs/namei.c | 8 ++++++++
fs/overlayfs/ovl_entry.h | 2 ++
fs/overlayfs/super.c | 16 ++++++++++------
3 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e9717c2f7d45..f47c591402d7 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -325,6 +325,14 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
int i;
for (i = 0; i < ofs->numlower; i++) {
+ /*
+ * If lower fs uuid is not unique among lower fs we cannot match
+ * fh->uuid to layer.
+ */
+ if (ofs->lower_layers[i].fsid &&
+ ofs->lower_layers[i].fs->bad_uuid)
+ continue;
+
origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt,
connected);
if (origin)
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index a8279280e88d..28348c44ea5b 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -22,6 +22,8 @@ struct ovl_config {
struct ovl_sb {
struct super_block *sb;
dev_t pseudo_dev;
+ /* Unusable (conflicting) uuid */
+ bool bad_uuid;
};
struct ovl_layer {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index afbcb116a7f1..5d4faab57ba0 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1255,17 +1255,18 @@ static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid)
{
unsigned int i;
- if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt))
- return true;
-
for (i = 0; i < ofs->numlowerfs; i++) {
/*
* We use uuid to associate an overlay lower file handle with a
* lower layer, so we can accept lower fs with null uuid as long
* as all lower layers with null uuid are on the same fs.
+ * if we detect multiple lower fs with the same uuid, we
+ * disable lower file handle decoding on all of them.
*/
- if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid))
+ if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid)) {
+ ofs->lower_fs[i].bad_uuid = true;
return false;
+ }
}
return true;
}
@@ -1277,6 +1278,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
unsigned int i;
dev_t dev;
int err;
+ bool bad_uuid = false;
/* fsid 0 is reserved for upper fs even with non upper overlay */
if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
@@ -1287,10 +1289,11 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
return i + 1;
}
- if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
+ if (ofs->upper_mnt && !ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
+ bad_uuid = true;
ofs->config.index = false;
ofs->config.nfs_export = false;
- pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
+ pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', enforcing index=off,nfs_export=off.\n",
uuid_is_null(&sb->s_uuid) ? "null" : "conflicting",
path->dentry);
}
@@ -1303,6 +1306,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
ofs->lower_fs[ofs->numlowerfs].sb = sb;
ofs->lower_fs[ofs->numlowerfs].pseudo_dev = dev;
+ ofs->lower_fs[ofs->numlowerfs].bad_uuid = bad_uuid;
ofs->numlowerfs++;
return ofs->numlowerfs;
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH][V2] ovl: fix lookup failure on multi lower squashfs
2019-11-07 10:49 [PATCH][V2] ovl: fix lookup failure on multi lower squashfs Colin King
@ 2019-11-12 9:02 ` Colin Ian King
2019-11-13 16:02 ` Miklos Szeredi
1 sibling, 0 replies; 7+ messages in thread
From: Colin Ian King @ 2019-11-12 9:02 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein, linux-unionfs, stable
Cc: kernel-janitors, linux-kernel
Hi there,
I was wondering if this fix will get applied?
Colin
On 07/11/2019 10:49, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the past, overlayfs required that lower fs have non null uuid in
> order to support nfs export and decode copy up origin file handles.
>
> Commit 9df085f3c9a2 ("ovl: relax requirement for non null uuid of
> lower fs") relaxed this requirement for nfs export support, as long
> as uuid (even if null) is unique among all lower fs.
>
> However, said commit unintentionally also relaxed the non null uuid
> requirement for decoding copy up origin file handles, regardless of
> the unique uuid requirement.
>
> Amend this mistake by disabling decoding of copy up origin file handle
> from lower fs with a conflicting uuid.
>
> We still encode copy up origin file handles from those fs, because
> file handles like those already exist in the wild and because they
> might provide useful information in the future.
>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Link: https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
> Fixes: 9df085f3c9a2 ("ovl: relax requirement for non null uuid ...")
> Cc: stable@vger.kernel.org # v4.20+
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> fs/overlayfs/namei.c | 8 ++++++++
> fs/overlayfs/ovl_entry.h | 2 ++
> fs/overlayfs/super.c | 16 ++++++++++------
> 3 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index e9717c2f7d45..f47c591402d7 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -325,6 +325,14 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> int i;
>
> for (i = 0; i < ofs->numlower; i++) {
> + /*
> + * If lower fs uuid is not unique among lower fs we cannot match
> + * fh->uuid to layer.
> + */
> + if (ofs->lower_layers[i].fsid &&
> + ofs->lower_layers[i].fs->bad_uuid)
> + continue;
> +
> origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt,
> connected);
> if (origin)
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index a8279280e88d..28348c44ea5b 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -22,6 +22,8 @@ struct ovl_config {
> struct ovl_sb {
> struct super_block *sb;
> dev_t pseudo_dev;
> + /* Unusable (conflicting) uuid */
> + bool bad_uuid;
> };
>
> struct ovl_layer {
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index afbcb116a7f1..5d4faab57ba0 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1255,17 +1255,18 @@ static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid)
> {
> unsigned int i;
>
> - if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt))
> - return true;
> -
> for (i = 0; i < ofs->numlowerfs; i++) {
> /*
> * We use uuid to associate an overlay lower file handle with a
> * lower layer, so we can accept lower fs with null uuid as long
> * as all lower layers with null uuid are on the same fs.
> + * if we detect multiple lower fs with the same uuid, we
> + * disable lower file handle decoding on all of them.
> */
> - if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid))
> + if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid)) {
> + ofs->lower_fs[i].bad_uuid = true;
> return false;
> + }
> }
> return true;
> }
> @@ -1277,6 +1278,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> unsigned int i;
> dev_t dev;
> int err;
> + bool bad_uuid = false;
>
> /* fsid 0 is reserved for upper fs even with non upper overlay */
> if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
> @@ -1287,10 +1289,11 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> return i + 1;
> }
>
> - if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> + if (ofs->upper_mnt && !ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> + bad_uuid = true;
> ofs->config.index = false;
> ofs->config.nfs_export = false;
> - pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
> + pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', enforcing index=off,nfs_export=off.\n",
> uuid_is_null(&sb->s_uuid) ? "null" : "conflicting",
> path->dentry);
> }
> @@ -1303,6 +1306,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
>
> ofs->lower_fs[ofs->numlowerfs].sb = sb;
> ofs->lower_fs[ofs->numlowerfs].pseudo_dev = dev;
> + ofs->lower_fs[ofs->numlowerfs].bad_uuid = bad_uuid;
> ofs->numlowerfs++;
>
> return ofs->numlowerfs;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][V2] ovl: fix lookup failure on multi lower squashfs
2019-11-07 10:49 [PATCH][V2] ovl: fix lookup failure on multi lower squashfs Colin King
2019-11-12 9:02 ` Colin Ian King
@ 2019-11-13 16:02 ` Miklos Szeredi
2019-11-13 16:34 ` Amir Goldstein
1 sibling, 1 reply; 7+ messages in thread
From: Miklos Szeredi @ 2019-11-13 16:02 UTC (permalink / raw)
To: Colin King
Cc: Miklos Szeredi, Amir Goldstein, overlayfs, stable,
kernel-janitors, linux-kernel
On Thu, Nov 7, 2019 at 11:50 AM Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> In the past, overlayfs required that lower fs have non null uuid in
> order to support nfs export and decode copy up origin file handles.
>
> Commit 9df085f3c9a2 ("ovl: relax requirement for non null uuid of
> lower fs") relaxed this requirement for nfs export support, as long
> as uuid (even if null) is unique among all lower fs.
>
> However, said commit unintentionally also relaxed the non null uuid
> requirement for decoding copy up origin file handles, regardless of
> the unique uuid requirement.
>
> Amend this mistake by disabling decoding of copy up origin file handle
> from lower fs with a conflicting uuid.
>
> We still encode copy up origin file handles from those fs, because
> file handles like those already exist in the wild and because they
> might provide useful information in the future.
>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Link: https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
> Fixes: 9df085f3c9a2 ("ovl: relax requirement for non null uuid ...")
> Cc: stable@vger.kernel.org # v4.20+
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> fs/overlayfs/namei.c | 8 ++++++++
> fs/overlayfs/ovl_entry.h | 2 ++
> fs/overlayfs/super.c | 16 ++++++++++------
> 3 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index e9717c2f7d45..f47c591402d7 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -325,6 +325,14 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> int i;
>
> for (i = 0; i < ofs->numlower; i++) {
> + /*
> + * If lower fs uuid is not unique among lower fs we cannot match
> + * fh->uuid to layer.
> + */
> + if (ofs->lower_layers[i].fsid &&
> + ofs->lower_layers[i].fs->bad_uuid)
> + continue;
> +
> origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt,
> connected);
> if (origin)
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index a8279280e88d..28348c44ea5b 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -22,6 +22,8 @@ struct ovl_config {
> struct ovl_sb {
> struct super_block *sb;
> dev_t pseudo_dev;
> + /* Unusable (conflicting) uuid */
> + bool bad_uuid;
> };
>
> struct ovl_layer {
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index afbcb116a7f1..5d4faab57ba0 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1255,17 +1255,18 @@ static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid)
> {
> unsigned int i;
>
> - if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt))
> - return true;
> -
> for (i = 0; i < ofs->numlowerfs; i++) {
> /*
> * We use uuid to associate an overlay lower file handle with a
> * lower layer, so we can accept lower fs with null uuid as long
> * as all lower layers with null uuid are on the same fs.
> + * if we detect multiple lower fs with the same uuid, we
> + * disable lower file handle decoding on all of them.
> */
> - if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid))
> + if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid)) {
> + ofs->lower_fs[i].bad_uuid = true;
> return false;
> + }
> }
> return true;
> }
> @@ -1277,6 +1278,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> unsigned int i;
> dev_t dev;
> int err;
> + bool bad_uuid = false;
>
> /* fsid 0 is reserved for upper fs even with non upper overlay */
> if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
> @@ -1287,10 +1289,11 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> return i + 1;
> }
>
> - if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> + if (ofs->upper_mnt && !ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
This seems bogus: why only check conflicting lower layers if there's
an upper layer?
> + bad_uuid = true;
> ofs->config.index = false;
> ofs->config.nfs_export = false;
> - pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
> + pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', enforcing index=off,nfs_export=off.\n",
And this while this makes sense, it doesn't really fit into this patch
(no change of behavior regarding how index and nfs_export are
handled).
Thanks,
Miklos
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][V2] ovl: fix lookup failure on multi lower squashfs
2019-11-13 16:02 ` Miklos Szeredi
@ 2019-11-13 16:34 ` Amir Goldstein
2019-11-14 14:12 ` Miklos Szeredi
0 siblings, 1 reply; 7+ messages in thread
From: Amir Goldstein @ 2019-11-13 16:34 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Colin King, Miklos Szeredi, overlayfs, stable, kernel-janitors,
linux-kernel
On Wed, Nov 13, 2019 at 6:02 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, Nov 7, 2019 at 11:50 AM Colin King <colin.king@canonical.com> wrote:
> >
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > In the past, overlayfs required that lower fs have non null uuid in
> > order to support nfs export and decode copy up origin file handles.
> >
> > Commit 9df085f3c9a2 ("ovl: relax requirement for non null uuid of
> > lower fs") relaxed this requirement for nfs export support, as long
> > as uuid (even if null) is unique among all lower fs.
> >
> > However, said commit unintentionally also relaxed the non null uuid
> > requirement for decoding copy up origin file handles, regardless of
> > the unique uuid requirement.
> >
> > Amend this mistake by disabling decoding of copy up origin file handle
> > from lower fs with a conflicting uuid.
> >
> > We still encode copy up origin file handles from those fs, because
> > file handles like those already exist in the wild and because they
> > might provide useful information in the future.
> >
> > Reported-by: Colin Ian King <colin.king@canonical.com>
> > Link: https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
> > Fixes: 9df085f3c9a2 ("ovl: relax requirement for non null uuid ...")
> > Cc: stable@vger.kernel.org # v4.20+
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > fs/overlayfs/namei.c | 8 ++++++++
> > fs/overlayfs/ovl_entry.h | 2 ++
> > fs/overlayfs/super.c | 16 ++++++++++------
> > 3 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index e9717c2f7d45..f47c591402d7 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -325,6 +325,14 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> > int i;
> >
> > for (i = 0; i < ofs->numlower; i++) {
> > + /*
> > + * If lower fs uuid is not unique among lower fs we cannot match
> > + * fh->uuid to layer.
> > + */
> > + if (ofs->lower_layers[i].fsid &&
> > + ofs->lower_layers[i].fs->bad_uuid)
> > + continue;
> > +
> > origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt,
> > connected);
> > if (origin)
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index a8279280e88d..28348c44ea5b 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -22,6 +22,8 @@ struct ovl_config {
> > struct ovl_sb {
> > struct super_block *sb;
> > dev_t pseudo_dev;
> > + /* Unusable (conflicting) uuid */
> > + bool bad_uuid;
> > };
> >
> > struct ovl_layer {
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index afbcb116a7f1..5d4faab57ba0 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -1255,17 +1255,18 @@ static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid)
> > {
> > unsigned int i;
> >
> > - if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt))
> > - return true;
> > -
Colin, I mislead you, this should be (I think):
if (!ofs->config.nfs_export && !ofs->upper_mnt)
return true;
> > for (i = 0; i < ofs->numlowerfs; i++) {
> > /*
> > * We use uuid to associate an overlay lower file handle with a
> > * lower layer, so we can accept lower fs with null uuid as long
> > * as all lower layers with null uuid are on the same fs.
> > + * if we detect multiple lower fs with the same uuid, we
> > + * disable lower file handle decoding on all of them.
> > */
> > - if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid))
> > + if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid)) {
> > + ofs->lower_fs[i].bad_uuid = true;
> > return false;
> > + }
> > }
> > return true;
> > }
> > @@ -1277,6 +1278,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> > unsigned int i;
> > dev_t dev;
> > int err;
> > + bool bad_uuid = false;
> >
> > /* fsid 0 is reserved for upper fs even with non upper overlay */
> > if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
> > @@ -1287,10 +1289,11 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> > return i + 1;
> > }
> >
> > - if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> > + if (ofs->upper_mnt && !ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
>
> This seems bogus: why only check conflicting lower layers if there's
> an upper layer?
It is bogus - it was my (wrong) suggestion.
The thinking was that we only decode origin fh if we have an upper layer
and index only valid with upper layer.
I forgot the case of nfs_export and lower-only setup.
Suggested fix above.
>
> > + bad_uuid = true;
> > ofs->config.index = false;
> > ofs->config.nfs_export = false;
> > - pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
> > + pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', enforcing index=off,nfs_export=off.\n",
>
> And this while this makes sense, it doesn't really fit into this patch
> (no change of behavior regarding how index and nfs_export are
> handled).
>
Again, this was my (not wrong?) suggestion.
What this patch changes is that ovl_lower_uuid_ok() can now return false
and we get to this print although user did not ask for index nor nfs_export.
So the "falling back" language no longer makes sense.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][V2] ovl: fix lookup failure on multi lower squashfs
2019-11-13 16:34 ` Amir Goldstein
@ 2019-11-14 14:12 ` Miklos Szeredi
2019-11-14 14:37 ` Amir Goldstein
0 siblings, 1 reply; 7+ messages in thread
From: Miklos Szeredi @ 2019-11-14 14:12 UTC (permalink / raw)
To: Amir Goldstein
Cc: Colin King, Miklos Szeredi, overlayfs, stable, kernel-janitors,
linux-kernel
On Wed, Nov 13, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Nov 13, 2019 at 6:02 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Thu, Nov 7, 2019 at 11:50 AM Colin King <colin.king@canonical.com> wrote:
> > >
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > In the past, overlayfs required that lower fs have non null uuid in
> > > order to support nfs export and decode copy up origin file handles.
> > >
> > > Commit 9df085f3c9a2 ("ovl: relax requirement for non null uuid of
> > > lower fs") relaxed this requirement for nfs export support, as long
> > > as uuid (even if null) is unique among all lower fs.
> > >
> > > However, said commit unintentionally also relaxed the non null uuid
> > > requirement for decoding copy up origin file handles, regardless of
> > > the unique uuid requirement.
> > >
> > > Amend this mistake by disabling decoding of copy up origin file handle
> > > from lower fs with a conflicting uuid.
> > >
> > > We still encode copy up origin file handles from those fs, because
> > > file handles like those already exist in the wild and because they
> > > might provide useful information in the future.
> > >
> > > Reported-by: Colin Ian King <colin.king@canonical.com>
> > > Link: https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
> > > Fixes: 9df085f3c9a2 ("ovl: relax requirement for non null uuid ...")
> > > Cc: stable@vger.kernel.org # v4.20+
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > > fs/overlayfs/namei.c | 8 ++++++++
> > > fs/overlayfs/ovl_entry.h | 2 ++
> > > fs/overlayfs/super.c | 16 ++++++++++------
> > > 3 files changed, 20 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > > index e9717c2f7d45..f47c591402d7 100644
> > > --- a/fs/overlayfs/namei.c
> > > +++ b/fs/overlayfs/namei.c
> > > @@ -325,6 +325,14 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> > > int i;
> > >
> > > for (i = 0; i < ofs->numlower; i++) {
> > > + /*
> > > + * If lower fs uuid is not unique among lower fs we cannot match
> > > + * fh->uuid to layer.
> > > + */
> > > + if (ofs->lower_layers[i].fsid &&
> > > + ofs->lower_layers[i].fs->bad_uuid)
> > > + continue;
> > > +
> > > origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt,
> > > connected);
> > > if (origin)
> > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > > index a8279280e88d..28348c44ea5b 100644
> > > --- a/fs/overlayfs/ovl_entry.h
> > > +++ b/fs/overlayfs/ovl_entry.h
> > > @@ -22,6 +22,8 @@ struct ovl_config {
> > > struct ovl_sb {
> > > struct super_block *sb;
> > > dev_t pseudo_dev;
> > > + /* Unusable (conflicting) uuid */
> > > + bool bad_uuid;
> > > };
> > >
> > > struct ovl_layer {
> > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > > index afbcb116a7f1..5d4faab57ba0 100644
> > > --- a/fs/overlayfs/super.c
> > > +++ b/fs/overlayfs/super.c
> > > @@ -1255,17 +1255,18 @@ static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid)
> > > {
> > > unsigned int i;
> > >
> > > - if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt))
> > > - return true;
> > > -
>
> Colin, I mislead you, this should be (I think):
>
> if (!ofs->config.nfs_export && !ofs->upper_mnt)
> return true;
>
> > > for (i = 0; i < ofs->numlowerfs; i++) {
> > > /*
> > > * We use uuid to associate an overlay lower file handle with a
> > > * lower layer, so we can accept lower fs with null uuid as long
> > > * as all lower layers with null uuid are on the same fs.
> > > + * if we detect multiple lower fs with the same uuid, we
> > > + * disable lower file handle decoding on all of them.
> > > */
> > > - if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid))
> > > + if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid)) {
> > > + ofs->lower_fs[i].bad_uuid = true;
> > > return false;
> > > + }
> > > }
> > > return true;
> > > }
> > > @@ -1277,6 +1278,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> > > unsigned int i;
> > > dev_t dev;
> > > int err;
> > > + bool bad_uuid = false;
> > >
> > > /* fsid 0 is reserved for upper fs even with non upper overlay */
> > > if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
> > > @@ -1287,10 +1289,11 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> > > return i + 1;
> > > }
> > >
> > > - if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> > > + if (ofs->upper_mnt && !ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> >
> > This seems bogus: why only check conflicting lower layers if there's
> > an upper layer?
>
> It is bogus - it was my (wrong) suggestion.
> The thinking was that we only decode origin fh if we have an upper layer
> and index only valid with upper layer.
> I forgot the case of nfs_export and lower-only setup.
> Suggested fix above.
>
> >
> > > + bad_uuid = true;
> > > ofs->config.index = false;
> > > ofs->config.nfs_export = false;
> > > - pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
> > > + pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', enforcing index=off,nfs_export=off.\n",
> >
> > And this while this makes sense, it doesn't really fit into this patch
> > (no change of behavior regarding how index and nfs_export are
> > handled).
> >
>
> Again, this was my (not wrong?) suggestion.
> What this patch changes is that ovl_lower_uuid_ok() can now return false
> and we get to this print although user did not ask for index nor nfs_export.
> So the "falling back" language no longer makes sense.
But does "enforcing" makes sense in this light? That's not what the
detected bad_uuid condition is about, it's about failing to utilize
origin markings to make inode numbers persistent for filesystems that
have null uuid. Is that correct? Can we do a message that makes
that somewhat more clearer?
Thanks,
Miklos
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][V2] ovl: fix lookup failure on multi lower squashfs
2019-11-14 14:12 ` Miklos Szeredi
@ 2019-11-14 14:37 ` Amir Goldstein
2019-11-14 15:34 ` Miklos Szeredi
0 siblings, 1 reply; 7+ messages in thread
From: Amir Goldstein @ 2019-11-14 14:37 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Colin King, Miklos Szeredi, overlayfs, stable, kernel-janitors,
linux-kernel
On Thu, Nov 14, 2019 at 4:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, Nov 13, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Nov 13, 2019 at 6:02 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Thu, Nov 7, 2019 at 11:50 AM Colin King <colin.king@canonical.com> wrote:
> > > >
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > In the past, overlayfs required that lower fs have non null uuid in
> > > > order to support nfs export and decode copy up origin file handles.
> > > >
> > > > Commit 9df085f3c9a2 ("ovl: relax requirement for non null uuid of
> > > > lower fs") relaxed this requirement for nfs export support, as long
> > > > as uuid (even if null) is unique among all lower fs.
> > > >
> > > > However, said commit unintentionally also relaxed the non null uuid
> > > > requirement for decoding copy up origin file handles, regardless of
> > > > the unique uuid requirement.
> > > >
> > > > Amend this mistake by disabling decoding of copy up origin file handle
> > > > from lower fs with a conflicting uuid.
> > > >
> > > > We still encode copy up origin file handles from those fs, because
> > > > file handles like those already exist in the wild and because they
> > > > might provide useful information in the future.
> > > >
> > > > Reported-by: Colin Ian King <colin.king@canonical.com>
> > > > Link: https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
> > > > Fixes: 9df085f3c9a2 ("ovl: relax requirement for non null uuid ...")
> > > > Cc: stable@vger.kernel.org # v4.20+
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > > fs/overlayfs/namei.c | 8 ++++++++
> > > > fs/overlayfs/ovl_entry.h | 2 ++
> > > > fs/overlayfs/super.c | 16 ++++++++++------
> > > > 3 files changed, 20 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > > > index e9717c2f7d45..f47c591402d7 100644
> > > > --- a/fs/overlayfs/namei.c
> > > > +++ b/fs/overlayfs/namei.c
> > > > @@ -325,6 +325,14 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> > > > int i;
> > > >
> > > > for (i = 0; i < ofs->numlower; i++) {
> > > > + /*
> > > > + * If lower fs uuid is not unique among lower fs we cannot match
> > > > + * fh->uuid to layer.
> > > > + */
> > > > + if (ofs->lower_layers[i].fsid &&
> > > > + ofs->lower_layers[i].fs->bad_uuid)
> > > > + continue;
> > > > +
> > > > origin = ovl_decode_real_fh(fh, ofs->lower_layers[i].mnt,
> > > > connected);
> > > > if (origin)
> > > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > > > index a8279280e88d..28348c44ea5b 100644
> > > > --- a/fs/overlayfs/ovl_entry.h
> > > > +++ b/fs/overlayfs/ovl_entry.h
> > > > @@ -22,6 +22,8 @@ struct ovl_config {
> > > > struct ovl_sb {
> > > > struct super_block *sb;
> > > > dev_t pseudo_dev;
> > > > + /* Unusable (conflicting) uuid */
> > > > + bool bad_uuid;
> > > > };
> > > >
> > > > struct ovl_layer {
> > > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > > > index afbcb116a7f1..5d4faab57ba0 100644
> > > > --- a/fs/overlayfs/super.c
> > > > +++ b/fs/overlayfs/super.c
> > > > @@ -1255,17 +1255,18 @@ static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid)
> > > > {
> > > > unsigned int i;
> > > >
> > > > - if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt))
> > > > - return true;
> > > > -
> >
> > Colin, I mislead you, this should be (I think):
> >
> > if (!ofs->config.nfs_export && !ofs->upper_mnt)
> > return true;
> >
> > > > for (i = 0; i < ofs->numlowerfs; i++) {
> > > > /*
> > > > * We use uuid to associate an overlay lower file handle with a
> > > > * lower layer, so we can accept lower fs with null uuid as long
> > > > * as all lower layers with null uuid are on the same fs.
> > > > + * if we detect multiple lower fs with the same uuid, we
> > > > + * disable lower file handle decoding on all of them.
> > > > */
> > > > - if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid))
> > > > + if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid)) {
> > > > + ofs->lower_fs[i].bad_uuid = true;
> > > > return false;
> > > > + }
> > > > }
> > > > return true;
> > > > }
> > > > @@ -1277,6 +1278,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> > > > unsigned int i;
> > > > dev_t dev;
> > > > int err;
> > > > + bool bad_uuid = false;
> > > >
> > > > /* fsid 0 is reserved for upper fs even with non upper overlay */
> > > > if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb == sb)
> > > > @@ -1287,10 +1289,11 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> > > > return i + 1;
> > > > }
> > > >
> > > > - if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> > > > + if (ofs->upper_mnt && !ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> > >
> > > This seems bogus: why only check conflicting lower layers if there's
> > > an upper layer?
> >
> > It is bogus - it was my (wrong) suggestion.
> > The thinking was that we only decode origin fh if we have an upper layer
> > and index only valid with upper layer.
> > I forgot the case of nfs_export and lower-only setup.
> > Suggested fix above.
> >
> > >
> > > > + bad_uuid = true;
> > > > ofs->config.index = false;
> > > > ofs->config.nfs_export = false;
> > > > - pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
> > > > + pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', enforcing index=off,nfs_export=off.\n",
> > >
> > > And this while this makes sense, it doesn't really fit into this patch
> > > (no change of behavior regarding how index and nfs_export are
> > > handled).
> > >
> >
> > Again, this was my (not wrong?) suggestion.
> > What this patch changes is that ovl_lower_uuid_ok() can now return false
> > and we get to this print although user did not ask for index nor nfs_export.
> > So the "falling back" language no longer makes sense.
>
> But does "enforcing" makes sense in this light? That's not what the
> detected bad_uuid condition is about, it's about failing to utilize
> origin markings to make inode numbers persistent for filesystems that
> have null uuid. Is that correct?
That is not exactly how I would describe bad_uuid.
ovl_lower_uuid_ok() had already existed for a while
it was requires for decoding lower file handles, which is
a requirement of both index and nfs_export.
What Colin has now reported brings to light the fact that
decoding lower file handles was also required for making inode
numbers persistent.
So the bad_uuid condition is required for all of the above, not
just for decoding origin.
> Can we do a message that makes
> that somewhat more clearer?
>
What about the logs:
pr_warn("overlayfs: upper fs does not support xattr,
falling back to index=off and metacopy=off.\n");
pr_warn("overlayfs: upper fs does not support file
handles, falling back to index=off.\n");
pr_warn("overlayfs: fs on '%s' does not support file
handles, falling back to index=off,nfs_export=off.\n",
Should we also change them to reflect the fact the decoding origin
is not supported???
Seems like a lot of hassle that will end up writing too much information
that most people won't understand.
IIRC, we also do not guaranty persistent inode numbers for hardlinks
when index=off.
As for the change in question (falling back => enforcing), if that bothers you,
we can get rid of this change by testing emitting the print only if
(ofs->config.nfs_export || ofs->config.index).
Thanks,
Amir.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][V2] ovl: fix lookup failure on multi lower squashfs
2019-11-14 14:37 ` Amir Goldstein
@ 2019-11-14 15:34 ` Miklos Szeredi
0 siblings, 0 replies; 7+ messages in thread
From: Miklos Szeredi @ 2019-11-14 15:34 UTC (permalink / raw)
To: Amir Goldstein
Cc: Colin King, Miklos Szeredi, overlayfs, stable, kernel-janitors,
linux-kernel
On Thu, Nov 14, 2019 at 3:37 PM Amir Goldstein <amir73il@gmail.com> wrote:
> What Colin has now reported brings to light the fact that
> decoding lower file handles was also required for making inode
> numbers persistent.
>
> So the bad_uuid condition is required for all of the above, not
> just for decoding origin.
>
> > Can we do a message that makes
> > that somewhat more clearer?
> >
>
> What about the logs:
>
> pr_warn("overlayfs: upper fs does not support xattr,
> falling back to index=off and metacopy=off.\n");
> pr_warn("overlayfs: upper fs does not support file
> handles, falling back to index=off.\n");
> pr_warn("overlayfs: fs on '%s' does not support file
> handles, falling back to index=off,nfs_export=off.\n",
>
> Should we also change them to reflect the fact the decoding origin
> is not supported???
>
> Seems like a lot of hassle that will end up writing too much information
> that most people won't understand.
>
> IIRC, we also do not guaranty persistent inode numbers for hardlinks
> when index=off.
>
> As for the change in question (falling back => enforcing), if that bothers you,
> we can get rid of this change by testing emitting the print only if
> (ofs->config.nfs_export || ofs->config.index).
That makes sense.
It would also make sense to have a section about inode number
persistence in the documentation.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-14 15:34 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-07 10:49 [PATCH][V2] ovl: fix lookup failure on multi lower squashfs Colin King
2019-11-12 9:02 ` Colin Ian King
2019-11-13 16:02 ` Miklos Szeredi
2019-11-13 16:34 ` Amir Goldstein
2019-11-14 14:12 ` Miklos Szeredi
2019-11-14 14:37 ` Amir Goldstein
2019-11-14 15:34 ` Miklos Szeredi
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).