Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Christian Schoenebeck <linux_oss@crudebyte.com>
To:     Dominique Martinet <asmadeus@codewreck.org>
Cc:     v9fs-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org,
        Eric Van Hensbergen <ericvh@gmail.com>,
        Latchesar Ionkov <lucho@ionkov.net>,
        David Howells <dhowells@redhat.com>
Subject: Re: [PATCH] net/9p: show warning on Tread/Twrite if wrong file mode
Date:   Thu, 16 Jun 2022 21:44:16 +0200
Message-ID: <1692377.rnsbsUYrV6@silver>
In-Reply-To: <E1o1tHC-00039k-04@lizzy.crudebyte.com>
References: <E1o1tHC-00039k-04@lizzy.crudebyte.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Donnerstag, 16. Juni 2022 19:09:42 CEST Christian Schoenebeck wrote:
> The netfs changes (eb497943fa21) introduced cases where 'Tread' was sent
> to 9p server on a fid that was opened in write-only file mode. It took
> some time to find the cause of the symptoms observed (EBADF errors in
> user space apps). Add warnings to detect such issues easier in future.
> 
> Signed-off-by: Christian Schoenebeck <linux_oss@crudebyte.com>
> Link: https://lore.kernel.org/netdev/3645230.Tf70N6zClz@silver/
> ---
> As requested by Dominique, here a clean version of my previous
> EBADF trap code to be merged. Dominique, if you already have an
> equivalent patch queued, then just go ahead. I don't mind.
> 
> I'm currently testing your EBADF fix patch and the discussed,
> slightly adjusted versions. Looking good so far, but I'll report
> back later on.
> 
> 
>  net/9p/client.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 8bba0d9cf975..05dead12702d 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -1555,6 +1555,8 @@ p9_client_read(struct p9_fid *fid, u64 offset, struct
> iov_iter *to, int *err) int total = 0;
>  	*err = 0;
> 
> +	WARN_ON((fid->mode & O_ACCMODE) == O_WRONLY);
> +
>  	while (iov_iter_count(to)) {
>  		int count;
> 
> @@ -1648,6 +1650,8 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct
> iov_iter *from, int *err) p9_debug(P9_DEBUG_9P, ">>> TWRITE fid %d offset
> %llu count %zd\n", fid->fid, offset, iov_iter_count(from));
> 
> +	WARN_ON((fid->mode & O_ACCMODE) == O_RDONLY);
> +
>  	while (iov_iter_count(from)) {
>  		int count = iov_iter_count(from);
>  		int rsize = fid->iounit;

Better postpone this patch for now: when I use cache=loose, everything looks
fine. But when I use cache=mmap it starts with the following warnings on boot:

[    7.164456] WARNING: CPU: 0 PID: 221 at net/9p/client.c:1653 p9_client_write+0x1b6/0x210 [9pnet]
[    7.164528] ? aa_replace_profiles (security/apparmor/policy.c:1089) 
[    7.164534] v9fs_file_write_iter (fs/9p/vfs_file.c:403) 9p
[    7.164539] new_sync_write (fs/read_write.c:505 (discriminator 1)) 
[    7.164551] vfs_write (fs/read_write.c:591) 
[    7.164557] ksys_write (fs/read_write.c:644) 
[    7.164559] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) 
[    7.164571] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)

[    9.698867] WARNING: CPU: 1 PID: 314 at net/9p/client.c:1653 p9_client_write+0x1b6/0x210 [9pnet]
[    9.737339] ? folio_add_lru (./arch/x86/include/asm/preempt.h:103 mm/swap.c:468) 
[    9.738599] ? _raw_spin_unlock (./arch/x86/include/asm/preempt.h:103 ./include/linux/spinlock_api_smp.h:143 kernel/locking/spinlock.c:186) 
[    9.739940] v9fs_file_write_iter (fs/9p/vfs_file.c:403) 9p
[    9.742655] new_sync_write (fs/read_write.c:505 (discriminator 1)) 
[    9.744063] vfs_write (fs/read_write.c:591) 
[    9.744858] ksys_write (fs/read_write.c:644) 
[    9.745573] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) 
[    9.746339] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)

And then after booting, when I start to actually do something on guest, it
spills the terminal with the following:

[  876.260885] WARNING: CPU: 1 PID: 197 at net/9p/client.c:1653 p9_client_write+0x1b6/0x210 [9pnet]
[  876.260955] ? preempt_count_add (./include/linux/ftrace.h:910 kernel/sched/core.c:5558 kernel/sched/core.c:5555 kernel/sched/core.c:5583) 
[  876.260960] v9fs_file_write_iter (fs/9p/vfs_file.c:403) 9p
[  876.260966] new_sync_write (fs/read_write.c:505 (discriminator 1)) 
[  876.260972] vfs_write (fs/read_write.c:591) 
[  876.260975] __x64_sys_pwrite64 (./include/linux/file.h:44 fs/read_write.c:707 fs/read_write.c:716 fs/read_write.c:713 fs/read_write.c:713) 
[  876.260979] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) 
[  876.260982] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)

Best regards,
Christian Schoenebeck