linux-kernel - Re: [PATCH] Always report a writeback error once

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: <20180423214348.GH13383@bombadil.infradead.org>
Date:   Mon, 23 Apr 2018 14:43:48 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Andres Freund <andres@...razel.de>
Cc:     linux-fsdevel@...r.kernel.org, Jeff Layton <jlayton@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Always report a writeback error once

On Mon, Apr 23, 2018 at 01:57:30PM -0700, Andres Freund wrote:
> On 2018-04-23 13:42:08 -0700, Matthew Wilcox wrote:
> > @@ -119,19 +119,11 @@ EXPORT_SYMBOL(errseq_set);
> >  errseq_t errseq_sample(errseq_t *eseq)
> >  {
> 
> There's a comment above this:
>  *
>  * This function allows callers to sample an errseq_t value, marking it as
>  * "seen" if required.

Oh, good catch.  I'll fix that.  Thanks!

> I've never really looked at this code in any depth before, but won't
> this potentially lead to the same error being reported on multiple FDs?
> Imagine two fds (potentially in different processes) getting the 0
> returned by errseq_sample() because it's not ERRSEQ_SEEN. Afaict
> file_check_and_advance_wb_err() will return an error that's always
> unlike 0 in that case, and thus the error will returned on both fds?
> 
> I'm personally perfectly fine with that, but it's not necessarily what's
> described as desired in your email?.

That's what I was trying to say with this paragraph:

> > This patch restores that behaviour by reporting errors to file descriptors
> > which are opened after the error occurred, but before it was reported
> > to any file descriptor.

Maybe it was a little unclear?  I'd appreciate a suggestion on making
it clearer.

I think this behaviour is perfectly justifiable.  Writeback errors occur
asynchronously to open.  Userspace can't tell the difference between:

kernel: writeback
p1: open
p2: open
p1: fsync
p2: fsync

and

p1: open
p2: open
kernel: writeback
p1: fsync
p2: fsync

Since both p1 and p2 would get the writeback error in the second case,
they can both get the writeback error in the first place.

p2 can't rely on this, after all we could have the sequence:

p1: open
p1: fsync
p2: open
p2: fsync

and p2 will not see the error, but it wouldn't have seen the error
before the errseq_t infrastructure went in.  And maybe p1 handled the
error three weeks ago; we don't want p2 to see the error.