| 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
| ||
|
Message-ID: <20231221230153.GA1607352@dev-arch.thelio-3990X>
Date: Thu, 21 Dec 2023 16:01:53 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: David Howells <dhowells@...hat.com>
Cc: Jeff Layton <jlayton@...nel.org>, Steve French <smfrench@...il.com>,
Matthew Wilcox <willy@...radead.org>,
Marc Dionne <marc.dionne@...istor.com>,
Paulo Alcantara <pc@...guebit.com>,
Shyam Prasad N <sprasad@...rosoft.com>, Tom Talpey <tom@...pey.com>,
Dominique Martinet <asmadeus@...ewreck.org>,
Eric Van Hensbergen <ericvh@...nel.org>,
Ilya Dryomov <idryomov@...il.com>,
Christian Brauner <christian@...uner.io>, linux-cachefs@...hat.com,
linux-afs@...ts.infradead.org, linux-cifs@...r.kernel.org,
linux-nfs@...r.kernel.org, ceph-devel@...r.kernel.org,
v9fs@...ts.linux.dev, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 37/40] netfs: Optimise away reads above the point at
which there can be no data
Hi David,
On Thu, Dec 21, 2023 at 01:23:32PM +0000, David Howells wrote:
> Track the file position above which the server is not expected to have any
> data (the "zero point") and preemptively assume that we can satisfy
> requests by filling them with zeroes locally rather than attempting to
> download them if they're over that line - even if we've written data back
> to the server. Assume that any data that was written back above that
> position is held in the local cache. Note that we have to split requests
> that straddle the line.
>
> Make use of this to optimise away some reads from the server. We need to
> set the zero point in the following circumstances:
>
> (1) When we see an extant remote inode and have no cache for it, we set
> the zero_point to i_size.
>
> (2) On local inode creation, we set zero_point to 0.
>
> (3) On local truncation down, we reduce zero_point to the new i_size if
> the new i_size is lower.
>
> (4) On local truncation up, we don't change zero_point.
>
> (5) On local modification, we don't change zero_point.
>
> (6) On remote invalidation, we set zero_point to the new i_size.
>
> (7) If stored data is discarded from the pagecache or culled from fscache,
> we must set zero_point above that if the data also got written to the
> server.
>
> (8) If dirty data is written back to the server, but not fscache, we must
> set zero_point above that.
>
> (9) If a direct I/O write is made, set zero_point above that.
>
> Assuming the above, any read from the server at or above the zero_point
> position will return all zeroes.
>
> The zero_point value can be stored in the cache, provided the above rules
> are applied to it by any code that culls part of the local cache.
>
> Signed-off-by: David Howells <dhowells@...hat.com>
> cc: Jeff Layton <jlayton@...nel.org>
> cc: linux-cachefs@...hat.com
> cc: linux-fsdevel@...r.kernel.org
> cc: linux-mm@...ck.org
> ---
<snip>
> diff --git a/include/linux/netfs.h b/include/linux/netfs.h
> index 8cde618cf6d9..a5374218efe4 100644
> --- a/include/linux/netfs.h
> +++ b/include/linux/netfs.h
> @@ -136,6 +136,8 @@ struct netfs_inode {
> struct fscache_cookie *cache;
> #endif
> loff_t remote_i_size; /* Size of the remote file */
> + loff_t zero_point; /* Size after which we assume there's no data
> + * on the server */
> unsigned long flags;
> #define NETFS_ICTX_ODIRECT 0 /* The file has DIO in progress */
> #define NETFS_ICTX_UNBUFFERED 1 /* I/O should not use the pagecache */
> @@ -463,22 +465,30 @@ static inline void netfs_inode_init(struct netfs_inode *ctx,
> {
> ctx->ops = ops;
> ctx->remote_i_size = i_size_read(&ctx->inode);
> + ctx->zero_point = ctx->remote_i_size;
> ctx->flags = 0;
> #if IS_ENABLED(CONFIG_FSCACHE)
> ctx->cache = NULL;
> #endif
> + /* ->releasepage() drives zero_point */
> + mapping_set_release_always(ctx->inode.i_mapping);
> }
I bisected a crash that I see when trying to mount an NFS volume to this
change as commit 6e3c8451f624 ("netfs: Optimise away reads above the
point at which there can be no data") in next-20231221:
[ 45.964963] BUG: kernel NULL pointer dereference, address: 0000000000000078
[ 45.964975] #PF: supervisor write access in kernel mode
[ 45.964982] #PF: error_code(0x0002) - not-present page
[ 45.964987] PGD 0 P4D 0
[ 45.964996] Oops: 0002 [#1] PREEMPT SMP NOPTI
[ 45.965004] CPU: 2 PID: 2419 Comm: mount.nfs Not tainted 6.7.0-rc6-next-20231221-debug-09925-g857647efa9be #1 adbbe7bc5037c662bc8f9b8e78ccf16be15b5e58
[ 45.965014] Hardware name: HP HP Desktop M01-F1xxx/87D6, BIOS F.12 12/17/2020
[ 45.965019] RIP: 0010:nfs_alloc_inode+0xa2/0xc0 [nfs]
[ 45.965092] Code: 80 b0 01 00 00 00 00 00 00 48 c7 80 38 04 00 00 00 f7 1e c2 48 c7 80 58 04 00 00 00 00 00 00 48 c7 80 40 04 00 00 00 00 00 00 <f0> 80 0a 80 48 05 b8 01 00 00 e9 5f 2b 20 f5 66 66 2e 0f 1f 84 00
[ 45.965099] RSP: 0018:ffffc900058f7bc0 EFLAGS: 00010286
[ 45.965107] RAX: ffff8881958c7290 RBX: ffff888168f0f800 RCX: 0000000000000000
[ 45.965112] RDX: 0000000000000078 RSI: ffffffffc2140a71 RDI: ffff88817a12b880
[ 45.965118] RBP: ffff888168f0f800 R08: ffffc900058f7b70 R09: 88728c958188ffff
[ 45.965123] R10: 000000000003a5c0 R11: 0000000000000005 R12: ffffffffc22f1a80
[ 45.965128] R13: ffffc900058f7c30 R14: 0000000000000000 R15: 0000000000000002
[ 45.965134] FS: 00007ff78c318740(0000) GS:ffff8887ff280000(0000) knlGS:0000000000000000
[ 45.965140] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 45.965146] CR2: 0000000000000078 CR3: 000000018a514000 CR4: 0000000000350ef0
[ 45.965152] Call Trace:
[ 45.965160] <TASK>
[ 45.965167] ? __die+0x23/0x70
[ 45.965183] ? page_fault_oops+0x173/0x4e0
[ 45.965197] ? nfs_alloc_inode+0x21/0xc0 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965279] ? exc_page_fault+0x7e/0x180
[ 45.965291] ? asm_exc_page_fault+0x26/0x30
[ 45.965308] ? nfs_alloc_inode+0x21/0xc0 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965374] ? nfs_alloc_inode+0xa2/0xc0 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965441] alloc_inode+0x1e/0xc0
[ 45.965452] ? __pfx_nfs_find_actor+0x10/0x10 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965517] iget5_locked+0x97/0xf0
[ 45.965525] ? __pfx_nfs_init_locked+0x10/0x10 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965593] nfs_fhget+0xe4/0x700 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965666] nfs_get_root+0xc6/0x4a0 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965732] ? kernfs_rename_ns+0x85/0x210
[ 45.965754] nfs_get_tree_common+0xc7/0x520 [nfs aac4a012b174ef6e5996d0df3638a0616e82eb47]
[ 45.965826] vfs_get_tree+0x29/0xf0
[ 45.965836] fc_mount+0x12/0x40
[ 45.965846] do_nfs4_mount+0x12e/0x370 [nfsv4 9bac1f2bd94d7294fbbaf875b7b5cec5adc527f5]
[ 45.965946] nfs4_try_get_tree+0x48/0xd0 [nfsv4 9bac1f2bd94d7294fbbaf875b7b5cec5adc527f5]
[ 45.966034] vfs_get_tree+0x29/0xf0
[ 45.966041] ? srso_return_thunk+0x5/0x5f
[ 45.966051] path_mount+0x4ca/0xb10
[ 45.966063] __x64_sys_mount+0x11a/0x150
[ 45.966074] do_syscall_64+0x64/0xe0
[ 45.966083] ? do_syscall_64+0x70/0xe0
[ 45.966090] ? syscall_exit_to_user_mode+0x2b/0x40
[ 45.966098] ? srso_return_thunk+0x5/0x5f
[ 45.966106] ? do_syscall_64+0x70/0xe0
[ 45.966113] ? srso_return_thunk+0x5/0x5f
[ 45.966121] ? exc_page_fault+0x7e/0x180
[ 45.966130] entry_SYSCALL_64_after_hwframe+0x6c/0x74
[ 45.966138] RIP: 0033:0x7ff78c5f2a1e
...
It appears that ctx->inode.i_mapping is NULL in netfs_inode_init(). This
patch appears to cure the problem for me but I am not sure if it is
proper or not.
Cheers,
Nathan
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index a5374218efe4..8daaba665421 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -471,7 +471,8 @@ static inline void netfs_inode_init(struct netfs_inode *ctx,
ctx->cache = NULL;
#endif
/* ->releasepage() drives zero_point */
- mapping_set_release_always(ctx->inode.i_mapping);
+ if (ctx->inode.i_mapping)
+ mapping_set_release_always(ctx->inode.i_mapping);
}
/**
Powered by blists - more mailing lists