linux-kernel - Re: [GIT PULL] overlayfs update for 4.9

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 14 Oct 2016 13:10:52 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Miklos Szeredi <miklos@...redi.hu>,
        Al Viro <viro@...iv.linux.org.uk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-unionfs@...r.kernel.org, fstests <fstests@...r.kernel.org>
Subject: Re: [GIT PULL] overlayfs update for 4.9

On Fri, Oct 14, 2016 at 9:48 AM, Amir Goldstein <amir73il@...il.com> wrote:
> On Fri, Oct 14, 2016 at 7:03 AM, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
>> On Thu, Oct 13, 2016 at 7:37 AM, Miklos Szeredi <miklos@...redi.hu> wrote:
>>>
>>> Please pull from:
>>
>> No.
>>
>> Or rather, I pulled and then immediately unpulled. When I look at the
>> diff, I saw an obvious bug in the very first chunk. I'm not going to
>> pull something that is this obviously buggy and untested.
>>
>> Your change to fs/ioctl.c to add a -EXDEV error case very clearly
>> leaks a reference to 'src_file'.
>
> On the charge of writing obviously buggy code I plead guilty :-/
> On the charge of not testing my code I plead not guilty.
> I exercised the FICLONERANGE intensively with the xfstests clone test group
> and never experienced any problem and any warning running with all
> relevant kernel debugging enabled.
>
> So how come this leak went unnoticed?
> Because fdget (__fget_light) does not take a reference if the fd is not shared.
>
> So what can we do to catch silly mistakes like this one earlier and
> without relying
> on Linus's spidy senses?
> Writing xfstests to test all fd interfaces with cloned fds? Is this a
> scalable solution?


Well, I added an idle loop thread to xfs_io and sure enough it catches the leak
in test generic/157 (Try cross-device reflink).
I will post patches to fstests.

Sorry...

> Or maybe it's best to add a trivial CONFIG_DEBUG_FDGET that will always
> skip the no refcount optimization?
>
> To me the second option seems preferred from engineering pov
> I can post this simple patch if you guys agree to this solution.
>
>>
>> It's too late in the merge window for this to be fixed up any more.
>> This has been a painful enough merge window for me that I'm not going
>> to play around with obviously buggy pull requests.
>>
>
> It has been a rocky merge window and I apologize for adding this last straw
> and pooping the party for the entire overlay pull request.
> In the hope that this may help to sweeten your verdict -
> I just got my hands on a brand new testing machine, which is dedicated
> to stress testing file systems on the latest rc.
>
> Amir.
>
>>                   Linus
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html