linux-kernel - Re: [PATCH] 9p: fix EBADF errors in cached mode

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: <YqiC8luskkxUftQl@codewreck.org>
Date:   Tue, 14 Jun 2022 21:45:38 +0900
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Christian Schoenebeck <linux_oss@...debyte.com>
Cc:     Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        David Howells <dhowells@...hat.com>,
        linux-fsdevel@...r.kernel.org, stable@...r.kernel.org,
        v9fs-developer@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] 9p: fix EBADF errors in cached mode

Christian Schoenebeck wrote on Tue, Jun 14, 2022 at 02:10:01PM +0200:
> It definitely goes into the right direction, but I think it's going a bit too 
> far by using writeback_fid also in cases where it is not necessary and wasn't 
> used before in the past.

Would help if I had an idea of what was used where in the past.. :)

>From a quick look at the code, checking out v5.10,
v9fs_vfs_writepage_locked() used the writeback fid always for all writes
v9fs_vfs_readpages is a bit more complex but only seems to be using the
"direct" private_data fid for reads...
It took me a bit of time but I think the reads you were seeing on
writeback fid come from v9fs_write_begin that does some readpage on the
writeback fid to populate the page before a non-filling write happens.

> What about something like this in v9fs_init_request() (yet untested):
> 
>     /* writeback_fid is always opened O_RDWR (instead of just O_WRONLY) 
>      * explicitly for this case: partial write backs that require a read
>      * prior to actual write and therefore requires a fid with read
>      * capability.
>      */
>     if (rreq->origin == NETFS_READ_FOR_WRITE)
>         fid = v9inode->writeback_fid;

... Which seems to be exactly what this origin is about, so if that
works I'm all for it.

> If desired, this could be further constrained later on like:
> 
>     if (rreq->origin == NETFS_READ_FOR_WRITE &&
>         (fid->mode & O_ACCMODE) == O_WRONLY)
>     {
>         fid = v9inode->writeback_fid;
>     }

That also makes sense, if the fid mode has read permissions we might as
well use these as the writeback fid would needlessly be doing root IOs.

> I will definitely give these options some test spins here, a short feedback 
> ahead would be appreciated though.

Please let me know how that works out, I'd be happy to use either of
your versions instead of mine.
If I can be greedy though I'd like to post it together with the other
couple of fixes next week, so having something before the end of the
week would be great -- I think even my first overkill version early and
building on it would make sense at this point.

But I think you've got the right end, so hopefully won't be needing to
delay


Cheers,
-- 
Dominique