[git pull] vfs fix

[git pull] vfs fix

[Al Viro]

Off-by-one in d_walk()/__dentry_kill() race fix.  It's very hard to hit;
possible in the same conditions as the original bug, except that you
need the skipped branch to contain all the remaining evictables, so that
the d_walk()-calling loop in d_invalidate() decides there's nothing more
to do and doesn't go for another pass - otherwise that next pass will
sweep the sucker.  So it's not too urgent, but seeing that the fix is obvious
and the original commit has spread into all -stable branches...
Please, pull from

git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

Shortlog:
Al Viro (1):
      d_walk() might skip too much

Diffstat:
 fs/dcache.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

[git pull] vfs fix

[Al Viro]

The following changes since commit 3ad0e83cf86bcaeb6ca3c37060a3ce866b25fb42:

  Merge branch 'parisc-4.9-4' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux (2016-11-25 16:47:15 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

for you to fetch changes up to 8e54cadab447dae779f80f79c87cbeaea9594f60:

  fix default_file_splice_read() (2016-11-26 20:05:42 -0500)

----------------------------------------------------------------
Al Viro (1):
      fix default_file_splice_read()

 fs/splice.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Re: [git pull] vfs fix

[Linus Torvalds]

On Sat, Nov 26, 2016 at 5:13 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Al Viro (1):
>       fix default_file_splice_read()

Ugh. I absolutely _hate_ this:

        BUG_ON(dummy);

because it makes no sense.

I'm assuming that "dummy" here is "start_offset", and that you want to
make sure that there are no initial offsets that would affect the
nrpages calculation.

But dammit, if so, just *call* it "start_offset", not "dummy". A dummy
value is just a place-holder, it makes no sense to have BUG_ON() on
such a value.

So adding random BUG_ON() statements is evil to begin with, but when
you do it on something that is mis-named and makes no sense, that's
just wrong.

I'm further assuming that the reason we can do that is because
"iov_iter_pipe()" has set iov_offset to zero, and as a result we end
up havin g

  iov_iter_get_pages_alloc() ->
    pipe_get_pages_alloc() ->
      data_start() will set *offp to zero.

but quite frankly, you can not tell that from the code itself, which
makes no sense. You have to go digging.

I was hoping the splice code would become more readable, not filled
with more crazy nonsensical code.

So I've pulled this, but _please_:

 - rename "dummy" (which isn't dummy at all now that you *do* things
to it!) to something sane.

   Like perhaps 'pg_offset' or 'iter_offset' or something.

 - Does the "BUG_ON()" really make sense? If the issue is that you
didn't use the offset in calculations, maybe you should just do so, ie
instead of

        BUG_ON(dummy);
        nr_pages = DIV_ROUND_UP(res, PAGE_SIZE);

   just do

        nr_pages = DIV_ROUND_UP(res + iter_offset, PAGE_SIZE);

or something? Even if "iter_offset" ends up always being zero, why is
that worthy of a BUG_ON()? The BUG_ON() is more expensive than just
doing the natural math..

That's what all the other users do, and that's what should be the
"right usage pattern", afaik. The number of pages really *is*
calculated as

       int n = DIV_ROUND_UP(result + offs, PAGE_SIZE);

in other iov_iter_get_pages_alloc() callers, although tghe nfs code
open-codes it as

        npages = (result + pgbase + PAGE_SIZE - 1) / PAGE_SIZE;

so it's not a very strong pattern.

                Linus

Re: [git pull] vfs fix

[Al Viro]

On Sat, Nov 26, 2016 at 05:48:54PM -0800, Linus Torvalds wrote:
> That's what all the other users do, and that's what should be the
> "right usage pattern", afaik. The number of pages really *is*
> calculated as
> 
>        int n = DIV_ROUND_UP(result + offs, PAGE_SIZE);
> 
> in other iov_iter_get_pages_alloc() callers, although tghe nfs code
> open-codes it as
> 
>         npages = (result + pgbase + PAGE_SIZE - 1) / PAGE_SIZE;
> 
> so it's not a very strong pattern.

Two issues here.  One is that iov_iter_get_pages{,_alloc}() calling
conventions are fucking ugly.  I'm guilty of that atrocity; my only
excuse is that this thing has congealed from many open-coded instances,
quite a few of those appearing only after considerable massage of the
code.  I _hate_ the boilerplate we have in the functions implementing
those for various iov_iter flavours and boilerplate in the callers.

I am going to try and come up with something less atrocious.  As it
is, renaming that variable and adding it to the return value of
iov_iter_get_pages_alloc() is certainly not a problem and would be
prettier, but TBH I just went "yet another place to go into that cleanup".
Shouldn't have.

Another thing is that it was a leftover from "Alexei, could you see if
that thing fixes your reproducer?" - just in case the things _really_ went
insane and it was not a wrong rounding but somehow completely buggered
pipe_get_pages_alloc().  They hadn't.

Anyway, leaving that BUG_ON() had been wrong; I can send a followup
massaging that thing as you've suggested, if you are interested in
that.  But keep in mind that the whole iov_iter_get_pages...() calling
conventions are going to be changed, hopefully soon.

Re: [git pull] vfs fix

[Al Viro]

On Sun, Nov 27, 2016 at 02:25:09AM +0000, Al Viro wrote:

> Anyway, leaving that BUG_ON() had been wrong; I can send a followup
> massaging that thing as you've suggested, if you are interested in
> that.  But keep in mind that the whole iov_iter_get_pages...() calling
> conventions are going to be changed, hopefully soon.

BTW, speaking of iov_iter_get_pages_alloc() callers and splice:
"ceph: combine as many iovec as possile into one OSD request" has added
static size_t dio_get_pagev_size(const struct iov_iter *it)
{
    const struct iovec *iov = it->iov;
    const struct iovec *iovend = iov + it->nr_segs;
    size_t size;

    size = iov->iov_len - it->iov_offset;
    /*
     * An iov can be page vectored when both the current tail
     * and the next base are page aligned.
     */
    while (PAGE_ALIGNED((iov->iov_base + iov->iov_len)) &&
           (++iov < iovend && PAGE_ALIGNED((iov->iov_base)))) {
        size += iov->iov_len;
    }
    dout("dio_get_pagevlen len = %zu\n", size);
    return size;
}

... with 'it' possibly being bio_vec-backed iterator.  Could somebody
explain what that code is trying to do?

I would really like to hear details - I'm not saying that the goal mentioned
in the commit message is worthless, but I don't know ceph codebase well
enough to tell what would be a good way to implement that.  In its current
form it's obviously broken.

Re: [git pull] vfs fix

[Al Viro]

On Sun, Nov 27, 2016 at 02:51:18AM +0000, Al Viro wrote:

... and screwed up Cc due to comma in S-O-B of commit in question.
My apologies.

Re: [git pull] vfs fix

[Yan, Zheng]

> On 27 Nov 2016, at 10:51, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> On Sun, Nov 27, 2016 at 02:25:09AM +0000, Al Viro wrote:
> 
>> Anyway, leaving that BUG_ON() had been wrong; I can send a followup
>> massaging that thing as you've suggested, if you are interested in
>> that.  But keep in mind that the whole iov_iter_get_pages...() calling
>> conventions are going to be changed, hopefully soon.
> 
> BTW, speaking of iov_iter_get_pages_alloc() callers and splice:
> "ceph: combine as many iovec as possile into one OSD request" has added
> static size_t dio_get_pagev_size(const struct iov_iter *it)
> {
>    const struct iovec *iov = it->iov;
>    const struct iovec *iovend = iov + it->nr_segs;
>    size_t size;
> 
>    size = iov->iov_len - it->iov_offset;
>    /*
>     * An iov can be page vectored when both the current tail
>     * and the next base are page aligned.
>     */
>    while (PAGE_ALIGNED((iov->iov_base + iov->iov_len)) &&
>           (++iov < iovend && PAGE_ALIGNED((iov->iov_base)))) {
>        size += iov->iov_len;
>    }
>    dout("dio_get_pagevlen len = %zu\n", size);
>    return size;
> }

The read/write interface for ceph cluster accepts page vector. But we can only specify offset for the first page. Except the first and last pages, all other pages must be full size. This function finds size of pages that satisfy this requirement

Regards
Yan, Zheng

> 
> ... with 'it' possibly being bio_vec-backed iterator.  Could somebody
> explain what that code is trying to do?
> 
> I would really like to hear details - I'm not saying that the goal mentioned
> in the commit message is worthless, but I don't know ceph codebase well
> enough to tell what would be a good way to implement that.  In its current
> form it's obviously broken.

Re: [git pull] vfs fix

[Linus Torvalds]

On Sat, Nov 26, 2016 at 6:25 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Two issues here.  One is that iov_iter_get_pages{,_alloc}() calling
> conventions are fucking ugly.

No arguments there. I was going to suggest adding an "int *npages"
argument and letting that function fill that in together with the page
array, but it's not like that is going to really make the interface
any prettier.

> Anyway, leaving that BUG_ON() had been wrong; I can send a followup
> massaging that thing as you've suggested, if you are interested in
> that.  But keep in mind that the whole iov_iter_get_pages...() calling
> conventions are going to be changed, hopefully soon.

If it indeed happens soon, I can certainly live with the existing code
as a temporary "ugly but it works". But if it ends up being post-4.10
I'd prefer to clean this up independently first, ok?

            Linus

[git pull] vfs fix

[Al Viro]

The following changes since commit f55532a0c0b8bb6148f4e07853b876ef73bc69ca:

  Linux 4.6-rc1 (2016-03-26 16:03:24 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

for you to fetch changes up to 7500c38ac3258815f86f41744a538850c3221b23:

  fix the braino in "namei: massage lookup_slow() to be usable by lookup_one_len_unlocked()" (2016-03-31 00:23:05 -0400)

----------------------------------------------------------------
Al Viro (1):
      fix the braino in "namei: massage lookup_slow() to be usable by lookup_one_len_unlocked()"

 fs/namei.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

[git pull] vfs fix

[Al Viro]

Really embarrassing braino; kudos to jlayton for catching that one.
Please, pull from
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

Al Viro (1):
      mount_subtree() pointless use-after-free

 fs/namespace.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Re: [git pull] vfs fix

[Linus Torvalds]

On Tue, Nov 22, 2011 at 9:44 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> Really embarrassing braino; kudos to jlayton for catching that one.
> Please, pull from
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus
>
> Al Viro (1):
>      mount_subtree() pointless use-after-free

Hmm. That's not at all what I get.

Or rather, that's just *one* of the commits I get. I also get these two:

      microblaze: bury asm/namei.h
      iio: fix a leak due to improper use of anon_inode_getfd()

which look like they may be valid, but for all I know you meant to
leave them for the next merge window. So not pulled. Please check your
tree,

                Linus

Re: [git pull] vfs fix

[Al Viro]

On Tue, Nov 22, 2011 at 09:48:36AM -0800, Linus Torvalds wrote:
> On Tue, Nov 22, 2011 at 9:44 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > Really embarrassing braino; kudos to jlayton for catching that one.
> > Please, pull from
> > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus
> >
> > Al Viro (1):
> > ? ? ?mount_subtree() pointless use-after-free
> 
> Hmm. That's not at all what I get.
> 
> Or rather, that's just *one* of the commits I get. I also get these two:
> 
>       microblaze: bury asm/namei.h
>       iio: fix a leak due to improper use of anon_inode_getfd()
> 
> which look like they may be valid, but for all I know you meant to
> leave them for the next merge window. So not pulled. Please check your
> tree,

Umm...  OK, I see what happened - buggered script generating
shortlog/diffstat plus me not noticing that it hadn't reported a couple
of fixes I'd put there yesterday.  All 3 are intended to be sent;
iio is an outright bugfix, microblaze one could wait, in theory, but
it's guaranteed to be safe (removal of header nothing includes, done on
all architectures years ago, with microblaze tree apparently copying it
from sparc and keeping all along) and the sooner such things are buried,
the fewer places will copy that *again*...

Anyway, consider that a pull request for all 3 commits.  Proper shortlog
and diffstat follow.

Shortlog:
Al Viro (3):
      microblaze: bury asm/namei.h
      iio: fix a leak due to improper use of anon_inode_getfd()
      mount_subtree() pointless use-after-free

Diffstat:
 arch/microblaze/include/asm/namei.h     |   22 ----------------------
 drivers/staging/iio/industrialio-core.c |   10 +++++++++-
 fs/namespace.c                          |    6 ++++--
 3 files changed, 13 insertions(+), 25 deletions(-)


FWIW, the fixed form of script follows; if you see any potential problems in
it, please tell:
echo Shortlog:
base=`git merge-base origin "$1"`
git log --pretty=short "$base..$1" | git shortlog
echo Diffstat:
git diff --stat -M "$base..$1"

Re: [git pull] vfs fix

[Linus Torvalds]

On Tue, Nov 22, 2011 at 1:09 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> FWIW, the fixed form of script follows; if you see any potential problems in
> it, please tell:

So not problems per se, just not very pretty.

> echo Shortlog:
> base=`git merge-base origin "$1"`
> git log --pretty=short "$base..$1" | git shortlog
> echo Diffstat:
> git diff --stat -M "$base..$1"

I would suggest this instead:

  echo Shortlog:
  git shortlog "origin..$1"
  echo Diffstat:
  git diff --stat --summary -M "origin...$1"

and skip all the merge-base crap (git will do it for you: with a "git
log" you don't need it, and with a "git diff" the three-dot version
will do it for you). And just use "git shortlog" directly.

And the above also uses "--summary" for the "git diff" to show a
summary of renames/deletes.

                        Linus

[git pull] vfs fix

[Al Viro]

I forgot to remove MAY_NOT_BLOCK from mask when we started to call
posix_acl_permission() on RCU path.  Ari has caught and fixed that.
Please, pull from
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git/ for-linus

Shortlog:
Ari Savolainen (1):
      Fix POSIX ACL permission check

Diffstat:
 fs/namei.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

[git pull] vfs fix

[Al Viro]

	Fix for very unpleasant braino in do_umount(); the effect was
that if umount(name, MNT_EXPIRE) failed with -EBUSY it left the kernel
with doubly-taken vfsmount_lock.  The next attempt to mount or umount
something would deadlock, of course...  Please, pull from

git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git/ for-linus

Shortlog:
J. R. Okajima (1):
      Unlock vfsmount_lock in do_umount

Diffstat:
 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)