Re: [GIT PULL] overlayfs update for 4.9

[Amir Goldstein]

On Fri, Oct 14, 2016 at 7:03 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Oct 13, 2016 at 7:37 AM, Miklos Szeredi <miklos@xxxxxxxxxx> 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?
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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html