| Message ID | 1526379972-20923-3-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 15, 2018 at 01:26:10PM +0300, Amir Goldstein wrote: > Overlayfs should cope with online changes to underlying layer > without crashing the kernel, which is what xfstest overlay/019 > checks. > > This test may sometimes trigger WARN_ON() in ovl_create_or_link() > when linking an overlay inode that has been changed on underlying > layer. > > Replace those WARN_ON() with pr_warn_ratelimited() to prevent > test from failing and because this is more appropriate to the > use case. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/overlayfs/dir.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index 62e6733b755c..25b339278684 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > if (!err) { > struct inode *realinode = d_inode(ovl_dentry_upper(dentry)); > > - WARN_ON(inode->i_mode != realinode->i_mode); > - WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid)); > - WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid)); > + if (inode->i_mode != realinode->i_mode || > + !uid_eq(inode->i_uid, realinode->i_uid) || > + !gid_eq(inode->i_gid, realinode->i_gid)) { > + pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n", > + dentry, inode->i_mode, > + from_kuid(&init_user_ns, inode->i_uid), > + from_kgid(&init_user_ns, inode->i_gid), > + realinode->i_mode, > + from_kuid(&init_user_ns, realinode->i_uid), > + from_kgid(&init_user_ns, realinode->i_gid)); Curious that why these calls to from_kuid() and from_kgid() was required in this patch. Vivek
On Tue, May 15, 2018 at 3:48 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > On Tue, May 15, 2018 at 01:26:10PM +0300, Amir Goldstein wrote: >> Overlayfs should cope with online changes to underlying layer >> without crashing the kernel, which is what xfstest overlay/019 >> checks. >> >> This test may sometimes trigger WARN_ON() in ovl_create_or_link() >> when linking an overlay inode that has been changed on underlying >> layer. >> >> Replace those WARN_ON() with pr_warn_ratelimited() to prevent >> test from failing and because this is more appropriate to the >> use case. >> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> --- >> fs/overlayfs/dir.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c >> index 62e6733b755c..25b339278684 100644 >> --- a/fs/overlayfs/dir.c >> +++ b/fs/overlayfs/dir.c >> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, >> if (!err) { >> struct inode *realinode = d_inode(ovl_dentry_upper(dentry)); >> >> - WARN_ON(inode->i_mode != realinode->i_mode); >> - WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid)); >> - WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid)); >> + if (inode->i_mode != realinode->i_mode || >> + !uid_eq(inode->i_uid, realinode->i_uid) || >> + !gid_eq(inode->i_gid, realinode->i_gid)) { >> + pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n", >> + dentry, inode->i_mode, >> + from_kuid(&init_user_ns, inode->i_uid), >> + from_kgid(&init_user_ns, inode->i_gid), >> + realinode->i_mode, >> + from_kuid(&init_user_ns, realinode->i_uid), >> + from_kgid(&init_user_ns, realinode->i_gid)); > > Curious that why these calls to from_kuid() and from_kgid() was required > in this patch. > kuid/kgid are opaque structs we should not access directly, so I thought this was the standard way to get a value for printing. Maybe I was wrong. Thanks, Amir.
On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote: > Overlayfs should cope with online changes to underlying layer > without crashing the kernel, which is what xfstest overlay/019 > checks. > > This test may sometimes trigger WARN_ON() in ovl_create_or_link() > when linking an overlay inode that has been changed on underlying > layer. > > Replace those WARN_ON() with pr_warn_ratelimited() to prevent > test from failing and because this is more appropriate to the > use case. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/overlayfs/dir.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index 62e6733b755c..25b339278684 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > if (!err) { > struct inode *realinode = d_inode(ovl_dentry_upper(dentry)); > > - WARN_ON(inode->i_mode != realinode->i_mode); > - WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid)); > - WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid)); > + if (inode->i_mode != realinode->i_mode || > + !uid_eq(inode->i_uid, realinode->i_uid) || > + !gid_eq(inode->i_gid, realinode->i_gid)) { > + pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n", > + dentry, inode->i_mode, > + from_kuid(&init_user_ns, inode->i_uid), > + from_kgid(&init_user_ns, inode->i_gid), > + realinode->i_mode, > + from_kuid(&init_user_ns, realinode->i_uid), > + from_kgid(&init_user_ns, realinode->i_gid)); > + } How about just dropping the warnings altogether. They didn't discover an issue with the code, just something normal, so IMO we should just get rid of them. Thanks, Miklois
On Wed, May 16, 2018 at 1:29 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> Overlayfs should cope with online changes to underlying layer >> without crashing the kernel, which is what xfstest overlay/019 >> checks. >> >> This test may sometimes trigger WARN_ON() in ovl_create_or_link() >> when linking an overlay inode that has been changed on underlying >> layer. >> >> Replace those WARN_ON() with pr_warn_ratelimited() to prevent >> test from failing and because this is more appropriate to the >> use case. >> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> --- >> fs/overlayfs/dir.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c >> index 62e6733b755c..25b339278684 100644 >> --- a/fs/overlayfs/dir.c >> +++ b/fs/overlayfs/dir.c >> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, >> if (!err) { >> struct inode *realinode = d_inode(ovl_dentry_upper(dentry)); >> >> - WARN_ON(inode->i_mode != realinode->i_mode); >> - WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid)); >> - WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid)); >> + if (inode->i_mode != realinode->i_mode || >> + !uid_eq(inode->i_uid, realinode->i_uid) || >> + !gid_eq(inode->i_gid, realinode->i_gid)) { >> + pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n", >> + dentry, inode->i_mode, >> + from_kuid(&init_user_ns, inode->i_uid), >> + from_kgid(&init_user_ns, inode->i_gid), >> + realinode->i_mode, >> + from_kuid(&init_user_ns, realinode->i_uid), >> + from_kgid(&init_user_ns, realinode->i_gid)); >> + } > > How about just dropping the warnings altogether. They didn't discover > an issue with the code, just something normal, so IMO we should just > get rid of them. > OK. On that subject, do you want to leave the 'debug' argument to ovl_do_XXX? I started peeling it off slowly from the new helpers, but maybe we should just yank it completely from the ovl_do_XXX helpers? pr_debug can be disabled dynamically anyway. right? Thanks, Amir.
On Wed, May 16, 2018 at 1:06 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Wed, May 16, 2018 at 1:29 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>> Overlayfs should cope with online changes to underlying layer >>> without crashing the kernel, which is what xfstest overlay/019 >>> checks. >>> >>> This test may sometimes trigger WARN_ON() in ovl_create_or_link() >>> when linking an overlay inode that has been changed on underlying >>> layer. >>> >>> Replace those WARN_ON() with pr_warn_ratelimited() to prevent >>> test from failing and because this is more appropriate to the >>> use case. >>> >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>> --- >>> fs/overlayfs/dir.c | 14 +++++++++++--- >>> 1 file changed, 11 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c >>> index 62e6733b755c..25b339278684 100644 >>> --- a/fs/overlayfs/dir.c >>> +++ b/fs/overlayfs/dir.c >>> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, >>> if (!err) { >>> struct inode *realinode = d_inode(ovl_dentry_upper(dentry)); >>> >>> - WARN_ON(inode->i_mode != realinode->i_mode); >>> - WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid)); >>> - WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid)); >>> + if (inode->i_mode != realinode->i_mode || >>> + !uid_eq(inode->i_uid, realinode->i_uid) || >>> + !gid_eq(inode->i_gid, realinode->i_gid)) { >>> + pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n", >>> + dentry, inode->i_mode, >>> + from_kuid(&init_user_ns, inode->i_uid), >>> + from_kgid(&init_user_ns, inode->i_gid), >>> + realinode->i_mode, >>> + from_kuid(&init_user_ns, realinode->i_uid), >>> + from_kgid(&init_user_ns, realinode->i_gid)); >>> + } >> >> How about just dropping the warnings altogether. They didn't discover >> an issue with the code, just something normal, so IMO we should just >> get rid of them. >> > > OK. On that subject, do you want to leave the 'debug' argument > to ovl_do_XXX? I started peeling it off slowly from the new helpers, > but maybe we should just yank it completely from the ovl_do_XXX > helpers? pr_debug can be disabled dynamically anyway. right? Right. The original idea was to not debug upper operations that are just verbatim copies of the overlay operation, but I guess it doesn't really hurt to debug unconditionally. Thanks, Miklos
On Wed, May 16, 2018 at 2:18 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Wed, May 16, 2018 at 1:06 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Wed, May 16, 2018 at 1:29 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>> On Tue, May 15, 2018 at 12:26 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>> Overlayfs should cope with online changes to underlying layer >>>> without crashing the kernel, which is what xfstest overlay/019 >>>> checks. >>>> >>>> This test may sometimes trigger WARN_ON() in ovl_create_or_link() >>>> when linking an overlay inode that has been changed on underlying >>>> layer. >>>> >>>> Replace those WARN_ON() with pr_warn_ratelimited() to prevent >>>> test from failing and because this is more appropriate to the >>>> use case. >>>> >>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>>> --- >>>> fs/overlayfs/dir.c | 14 +++++++++++--- >>>> 1 file changed, 11 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c >>>> index 62e6733b755c..25b339278684 100644 >>>> --- a/fs/overlayfs/dir.c >>>> +++ b/fs/overlayfs/dir.c >>>> @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, >>>> if (!err) { >>>> struct inode *realinode = d_inode(ovl_dentry_upper(dentry)); >>>> >>>> - WARN_ON(inode->i_mode != realinode->i_mode); >>>> - WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid)); >>>> - WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid)); >>>> + if (inode->i_mode != realinode->i_mode || >>>> + !uid_eq(inode->i_uid, realinode->i_uid) || >>>> + !gid_eq(inode->i_gid, realinode->i_gid)) { >>>> + pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n", >>>> + dentry, inode->i_mode, >>>> + from_kuid(&init_user_ns, inode->i_uid), >>>> + from_kgid(&init_user_ns, inode->i_gid), >>>> + realinode->i_mode, >>>> + from_kuid(&init_user_ns, realinode->i_uid), >>>> + from_kgid(&init_user_ns, realinode->i_gid)); >>>> + } >>> >>> How about just dropping the warnings altogether. They didn't discover >>> an issue with the code, just something normal, so IMO we should just >>> get rid of them. >>> >> I guess it would be wise to leave that check in at least for the case we end up using a cached inode instead of the new inode we created... Thanks, Amir.
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index 62e6733b755c..25b339278684 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -525,9 +525,17 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, if (!err) { struct inode *realinode = d_inode(ovl_dentry_upper(dentry)); - WARN_ON(inode->i_mode != realinode->i_mode); - WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid)); - WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid)); + if (inode->i_mode != realinode->i_mode || + !uid_eq(inode->i_uid, realinode->i_uid) || + !gid_eq(inode->i_gid, realinode->i_gid)) { + pr_warn_ratelimited("overlayfs: real inode attributes mismatch (%pd2, %o.%u.%u != %o.%u.%u).\n", + dentry, inode->i_mode, + from_kuid(&init_user_ns, inode->i_uid), + from_kgid(&init_user_ns, inode->i_gid), + realinode->i_mode, + from_kuid(&init_user_ns, realinode->i_uid), + from_kgid(&init_user_ns, realinode->i_gid)); + } } return err; }
Overlayfs should cope with online changes to underlying layer without crashing the kernel, which is what xfstest overlay/019 checks. This test may sometimes trigger WARN_ON() in ovl_create_or_link() when linking an overlay inode that has been changed on underlying layer. Replace those WARN_ON() with pr_warn_ratelimited() to prevent test from failing and because this is more appropriate to the use case. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/overlayfs/dir.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)