[PATCH] net/9p: show warning on Tread/Twrite if wrong file mode

[PATCH] net/9p: show warning on Tread/Twrite if wrong file mode

[Christian Schoenebeck]

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;
-- 
2.30.2

Re: [PATCH] net/9p: show warning on Tread/Twrite if wrong file mode

[Christian Schoenebeck]

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

Re: [PATCH] net/9p: show warning on Tread/Twrite if wrong file mode

[Dominique Martinet]

Christian Schoenebeck wrote on Thu, Jun 16, 2022 at 09:44:16PM +0200:
> 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:

Noted, I guess that means we're misusing some fids somewhere...

Will have a look when able
--
Dominique