| Message ID | 1491555701-16608-3-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote: > truncate an overlayfs inode was checking IS_APPEND() on > overlay inode, but overlay inode does not have the S_APPEND flag. > > Move the IS_APPEND() check to after we have the upperdentry > and pass it the real upper inode. Not sure the reordering is worth it. Just use d_real_inode() in the IS_APPEND() check. OTOH it shouldn't matter (well, except whether the file is copied up or not in the error caae ). But mainly I just feel when there's a choice of a simpler way we should use that. Also it's usually better not to mix fixes with cleanups (the d_inode() thing). Thanks, Miklos > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/open.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/open.c b/fs/open.c > index b7d5ab1..425db52 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -87,10 +87,6 @@ long vfs_truncate(const struct path *path, loff_t length) > if (error) > goto mnt_drop_write_and_out; > > - error = -EPERM; > - if (IS_APPEND(inode)) > - goto mnt_drop_write_and_out; > - > /* > * If this is an overlayfs then do as if opening the file so we get > * write access on the upper inode, not on the overlay inode. For > @@ -101,7 +97,11 @@ long vfs_truncate(const struct path *path, loff_t length) > if (IS_ERR(upperdentry)) > goto mnt_drop_write_and_out; > > - error = get_write_access(upperdentry->d_inode); > + error = -EPERM; > + if (IS_APPEND(d_inode(upperdentry))) > + goto mnt_drop_write_and_out; > + > + error = get_write_access(d_inode(upperdentry)); > if (error) > goto mnt_drop_write_and_out; > > @@ -120,7 +120,7 @@ long vfs_truncate(const struct path *path, loff_t length) > error = do_truncate(path->dentry, length, 0, NULL); > > put_write_and_out: > - put_write_access(upperdentry->d_inode); > + put_write_access(d_inode(upperdentry)); > mnt_drop_write_and_out: > mnt_drop_write(path->mnt); > out: > -- > 2.7.4 >
On Fri, Apr 7, 2017 at 4:21 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote: >> truncate an overlayfs inode was checking IS_APPEND() on >> overlay inode, but overlay inode does not have the S_APPEND flag. >> >> Move the IS_APPEND() check to after we have the upperdentry >> and pass it the real upper inode. > > Not sure the reordering is worth it. Just use d_real_inode() in the Hmm, I meant move after d_real() which actually does a copy up. If we use d_real_inode() before d_real() then we prevent truncate of a lower with chattr +a. That is not consistent at all with open(O_TRUNC) and ftruncate and with checks for IS_IMMUTABLE() which only test upper (and I think it is better this way). > IS_APPEND() check. OTOH it shouldn't matter (well, except whether the > file is copied up or not in the error caae ). But mainly I just feel > when there's a choice of a simpler way we should use that. > > Also it's usually better not to mix fixes with cleanups (the d_inode() thing). > Sure, I can split that or drop the cleanup altogether. > Thanks, > Miklos > >> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> --- >> fs/open.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/fs/open.c b/fs/open.c >> index b7d5ab1..425db52 100644 >> --- a/fs/open.c >> +++ b/fs/open.c >> @@ -87,10 +87,6 @@ long vfs_truncate(const struct path *path, loff_t length) >> if (error) >> goto mnt_drop_write_and_out; >> >> - error = -EPERM; >> - if (IS_APPEND(inode)) >> - goto mnt_drop_write_and_out; >> - >> /* >> * If this is an overlayfs then do as if opening the file so we get >> * write access on the upper inode, not on the overlay inode. For >> @@ -101,7 +97,11 @@ long vfs_truncate(const struct path *path, loff_t length) >> if (IS_ERR(upperdentry)) >> goto mnt_drop_write_and_out; >> >> - error = get_write_access(upperdentry->d_inode); >> + error = -EPERM; >> + if (IS_APPEND(d_inode(upperdentry))) >> + goto mnt_drop_write_and_out; >> + >> + error = get_write_access(d_inode(upperdentry)); >> if (error) >> goto mnt_drop_write_and_out; >> >> @@ -120,7 +120,7 @@ long vfs_truncate(const struct path *path, loff_t length) >> error = do_truncate(path->dentry, length, 0, NULL); >> >> put_write_and_out: >> - put_write_access(upperdentry->d_inode); >> + put_write_access(d_inode(upperdentry)); >> mnt_drop_write_and_out: >> mnt_drop_write(path->mnt); >> out: >> -- >> 2.7.4 >>
On Fri, Apr 7, 2017 at 7:16 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Fri, Apr 7, 2017 at 4:21 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> On Fri, Apr 7, 2017 at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote: >>> truncate an overlayfs inode was checking IS_APPEND() on >>> overlay inode, but overlay inode does not have the S_APPEND flag. >>> >>> Move the IS_APPEND() check to after we have the upperdentry >>> and pass it the real upper inode. >> >> Not sure the reordering is worth it. Just use d_real_inode() in the > > Hmm, I meant move after d_real() which actually does a copy up. > If we use d_real_inode() before d_real() then we prevent truncate > of a lower with chattr +a. > That is not consistent at all with open(O_TRUNC) and ftruncate > and with checks for IS_IMMUTABLE() which only test upper > (and I think it is better this way). > >> IS_APPEND() check. OTOH it shouldn't matter (well, except whether the >> file is copied up or not in the error caae ). But mainly I just feel >> when there's a choice of a simpler way we should use that. >> >> Also it's usually better not to mix fixes with cleanups (the d_inode() thing). >> > > Sure, I can split that or drop the cleanup altogether. > Or drop the patch altogether... d_real() already takes care of IS_APPEND() now :)
diff --git a/fs/open.c b/fs/open.c index b7d5ab1..425db52 100644 --- a/fs/open.c +++ b/fs/open.c @@ -87,10 +87,6 @@ long vfs_truncate(const struct path *path, loff_t length) if (error) goto mnt_drop_write_and_out; - error = -EPERM; - if (IS_APPEND(inode)) - goto mnt_drop_write_and_out; - /* * If this is an overlayfs then do as if opening the file so we get * write access on the upper inode, not on the overlay inode. For @@ -101,7 +97,11 @@ long vfs_truncate(const struct path *path, loff_t length) if (IS_ERR(upperdentry)) goto mnt_drop_write_and_out; - error = get_write_access(upperdentry->d_inode); + error = -EPERM; + if (IS_APPEND(d_inode(upperdentry))) + goto mnt_drop_write_and_out; + + error = get_write_access(d_inode(upperdentry)); if (error) goto mnt_drop_write_and_out; @@ -120,7 +120,7 @@ long vfs_truncate(const struct path *path, loff_t length) error = do_truncate(path->dentry, length, 0, NULL); put_write_and_out: - put_write_access(upperdentry->d_inode); + put_write_access(d_inode(upperdentry)); mnt_drop_write_and_out: mnt_drop_write(path->mnt); out:
truncate an overlayfs inode was checking IS_APPEND() on overlay inode, but overlay inode does not have the S_APPEND flag. Move the IS_APPEND() check to after we have the upperdentry and pass it the real upper inode. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/open.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)