From: Jan Stancek <jstancek@redhat.com>
To: mtk manpages <mtk.manpages@gmail.com>
Cc: Miklos Szeredi <mszeredi@redhat.com>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
viro <viro@zeniv.linux.org.uk>,
guaneryu@gmail.com, Cyril Hrubis <chrubis@suse.cz>,
ltp@lists.linux.it, Linux API <linux-api@vger.kernel.org>,
Dave Chinner <dchinner@redhat.com>
Subject: Re: utimensat EACCES vs. EPERM in 4.8+
Date: Tue, 17 Jan 2017 02:51:40 -0500 (EST) [thread overview]
Message-ID: <280513509.810300.1484639500985.JavaMail.zimbra@redhat.com> (raw)
In-Reply-To: <CAKgNAkjTkBMY0wrj3wsH39YF=bHp=8mbYrXkSPzn0X4ezfso1w@mail.gmail.com>
> >> we seem to have a conflict between kernel and man pages.
>
> Jan, thanks for spotting this.
Credit goes to Cyril. As for LTP-20170116 (released yesterday) we went
with current documented behavior - few failures are expected on 4.8+.
Thanks for detailed analysis Michael!
>
> >> From utimensat man page:
> >>
> >> EACCES times is NULL, or both tv_nsec values are UTIME_NOW, and either:
> >> * the effective user ID of the caller does not match the owner of
> >> the
> >> file, the caller does not have write access to the file,
> >> and the
> >> caller is not privileged (Linux: does not have either the
> >> CAP_FOWNER
> >> or the CAP_DAC_OVERRIDE capability); or,
> >> * the file is marked immutable (see chattr(1)).
> >>
> >> But following 2 commits gradually replaced EACCES with EPERM.
> >>
> >> commit 337684a1746f93ae107e05d90977b070bb7e39d8
> >> Author: Eryu Guan <guaneryu@gmail.com>
> >> Date: Tue Aug 2 19:58:28 2016 +0800
> >> fs: return EPERM on immutable inode
> >
> > I agree with Eryu that consistently returning EPERM for immutable is
> > better than sometimes returning EACCESS and sometimes EPERM.
>
> I'm not so sure about that. In Eryu's patch (which *really, really*
> should have CCed linux-api@, and it would be kind if subsystem
> maintainers reminded patch submitters about that), there was this
> change:
>
> [[
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -402,23 +402,23 @@ static inline int do_inode_permission(struct
> inode *inode, int mask)
> * inode_permission().
> */
> int __inode_permission(struct inode *inode, int mask)
> {
> int retval;
>
> if (unlikely(mask & MAY_WRITE)) {
> /*
> * Nobody gets write access to an immutable file.
> */
> if (IS_IMMUTABLE(inode))
> - return -EACCES;
> + return -EPERM;
>
> /*
> * Updating mtime will likely cause i_uid and i_gid to be
> * written back improperly if their true value is unknown
> * to the vfs.
> */
> if (HAS_UNMAPPED_ID(inode))
> return -EACCES;
> }
>
> retval = do_inode_permission(inode, mask);
> ]]
>
> [1] The effects of that change are pretty wide ranging, affecting
> open(2)/openat(2) (of an existing file for writing),
> access(2)/faccessat(2) (W_OK), and [f]truncate(2). In addition, there
> is the observed change (from another part of the patch) in
> utimensat(2) (and friends). Those cases formerly gave EACCES for
> immutable files, now they give EPERM.
>
> [2] By contrast, the following always gave EPERM: fallocate(2),
> setxattr(2), unlink(2), link(2) [in certain cases], chown(2),
> chmod(2), and some per-filesystem cases of operations such as
> truncate.
>
> > So I think the man page should be fixed.
>
> I agree that the inconsistency in the error return for immutable files
> is unfortunate. But, consider the following:
>
> * Although the set of calls in [1] is shorter, they (in particular,
> open(2)) are probably much more commonly used than
> the system calls in [2]. (That is, Eryu's statement "In most cases,
> EPERM is returned on immutable inode" that accompanied the
> kernel patch isn't correct.)
> * For access(W_OK), we introduced a new error (EPERM) that
> previously never previously occurred. If there are applications
> that use access() and check specific error returns, they'll be
> confused. (I acknowledge there may be few such applications.)
> * We changed the carefully documented behavior of utimensat(2)
> (and friends). [Read the man page!]
> * EACCES is the typical error for "file not writable" (because of file
> permissions or other reasons such as immutability). It's long
> been the behavior for open(O_WRONLY/O_RDWR) on immutable
> files; now that has changed.
> * Now various man pages need to document two different (kernel
> version dependent) errors for immutable files (for the syscalls in [1],
> above), and applications may need to deal with those two errors.
>
> Summary of the above list: there's a nontrivial risk that something in
> userspace got broken. (And just because we didn't hear about it yet
> doesn't mean it didn't happen; sometimes these reports only arrive
> many months or even years later.)
>
> So, (1) I'm struggling to see the rationale for this change (I don't
> think "consistency" is enough) and (2) if "consistency" is the
> argument then (because the set of system calls in [1] are more
> frequently used than those in [2]), there's a reasonable argument that
> the change should have gone the other way: changing all IS_IMMUTABLE
> cases to fail with EACCES.
>
> Summary: I think there's an argument for reverting the kernel patch.
>
> Cheers,
>
> Michael
>
> --
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
>
next prev parent reply other threads:[~2017-01-17 7:51 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-16 15:46 utimensat EACCES vs. EPERM in 4.8+ Jan Stancek
2017-01-16 15:53 ` Miklos Szeredi
2017-01-17 0:04 ` Michael Kerrisk (man-pages)
2017-01-17 4:50 ` Carlos O'Donell
2017-01-17 7:51 ` Jan Stancek [this message]
2017-01-17 7:57 ` Cyril Hrubis
2017-01-17 9:39 ` Miklos Szeredi
2017-01-17 15:43 ` Cyril Hrubis
2017-01-18 8:23 ` Michael Kerrisk (man-pages)
2017-01-31 12:09 ` Cyril Hrubis
2017-01-17 4:41 ` Theodore Ts'o
2017-01-17 19:35 ` J. Bruce Fields
2017-01-17 21:04 ` Theodore Ts'o
2017-01-18 8:17 ` Michael Kerrisk (man-pages)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=280513509.810300.1484639500985.JavaMail.zimbra@redhat.com \
--to=jstancek@redhat.com \
--cc=chrubis@suse.cz \
--cc=dchinner@redhat.com \
--cc=guaneryu@gmail.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=ltp@lists.linux.it \
--cc=mszeredi@redhat.com \
--cc=mtk.manpages@gmail.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).