linux-kernel - Re: Commit 'fs: Do not check if there is a fsnotify watcher on pseudo inodes' breaks chromium here

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxhdOMbn9vL_PAGKLtriVzkjwBkuEgbdB5+uH2ZM6uA97w@mail.gmail.com>
Date:   Mon, 29 Jun 2020 17:05:38 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Jan Kara <jack@...e.cz>
Cc:     Maxim Levitsky <mlevitsk@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: Commit 'fs: Do not check if there is a fsnotify watcher on pseudo
 inodes' breaks chromium here

On Mon, Jun 29, 2020 at 4:09 PM Jan Kara <jack@...e.cz> wrote:
>
> On Sun 28-06-20 15:53:51, Amir Goldstein wrote:
> > On Sun, Jun 28, 2020 at 2:14 PM Maxim Levitsky <mlevitsk@...hat.com> wrote:
> > >
> > > Hi,
> > >
> > > I just did usual kernel update and now chromium crashes on startup.
> > > It happens both in a KVM's VM (with virtio-gpu if that matters) and natively with amdgpu driver.
> > > Most likely not GPU related although I initially suspected that it is.
> > >
> > > Chromium starts as a white rectangle, shows few white rectangles
> > > that resemble its notifications and then crashes.
> > >
> > > The stdout output from chromium:
> >
> > I guess this answers our question whether we could disable fsnoitfy
> > watches on pseudo inodes....
>
> Right :-|
>
> > From comments like these in chromium code:
> > https://chromium.googlesource.com/chromium/src/+/master/mojo/core/watcher_dispatcher.cc#77
> > https://chromium.googlesource.com/chromium/src/+/master/base/files/file_descriptor_watcher_posix.cc#176
> > https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_mojo.cc#240
> >
> > I am taking a wild guess that the missing FS_CLOSE event on anonymous pipes is
> > the cause for regression.
>
> I was checking the Chromium code for some time. It uses inotify in
> base/files/file_path_watcher_linux.cc and watches IN_CLOSE_WRITE event
> (among other ones) but I was unable to track down how the class gets
> connected to the mojo class that crashes. I'd be somewhat curious how they
> place inotify watches on pipe inodes - probably they have to utilize proc
> magic links but I'd like to be sure. Anyway your guess appears to be
> correct :)

Well, I lost track of the code as well...

>
> > The motivation for the patch "fs: Do not check if there is a fsnotify
> > watcher on pseudo inodes"
> > was performance, but actually, FS_CLOSE and FS_OPEN events probably do
> > not impact performance as FS_MODIFY and FS_ACCESS.
>
> Correct.
>
> > Do you agree that dropping FS_MODIFY/FS_ACCESS events for FMODE_STREAM
> > files as a general rule should be safe?
>
> Hum, so your patch drops FS_MODIFY/FS_ACCESS events also for named pipes
> compared to the original patch AFAIU and for those fsnotify works fine
> so far. So I'm not sure we won't regress someone else with this.
>
> I've also tested inotify on a sample pipe like: cat /dev/stdin | tee
> and watched /proc/<tee pid>/fd/0 and it actually generated IN_MODIFY |
> IN_ACCESS when data arrived to a pipe and tee(1) read it and then
> IN_CLOSE_WRITE | IN_CLOSE_NOWRITE when the pipe got closed (I thought you
> mentioned modify and access events didn't get properly generated?).

I don't think that I did (did I?)

>
> So as much as I agree that some fsnotify events on FMODE_STREAM files are
> dubious, they could get used (possibly accidentally) and so after this
> Chromium experience I think we just have to revert the change and live with
> generating notification events for pipes to avoid userspace regressions.
>
> Thoughts?

I am fine with that.

Before I thought of trying out FMODE_STREAM I was considering to propose
to set the new flag FMODE_NOIONOTIFY in alloc_file_pseudo() to narrow Mel's
patch to dropping FS_MODIFY|FS_ACCESS.

But I guess the burden of proof is back on Mel.
And besides, quoting Mel's patch:
"A patch is pending that reduces, but does not eliminate, the overhead of
    fsnotify but for files that cannot be looked up via a path, even that
    small overhead is unnecessary"

So really, we are not even sacrificing much by reverting this patch.
We down to "nano optimizations".

Thanks,
Amir.