> On Jan 9, 2023, at 10:26 AM, Anna Schumaker <schumaker.anna@xxxxxxxxx> wrote: > > On Mon, Jan 9, 2023 at 10:12 AM Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote: >> >> >> >>> On Jan 9, 2023, at 9:44 AM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: >>> >>> >>> >>>> On Jan 9, 2023, at 03:42, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: >>>> >>>> On 09/01/2023 09:14, Krzysztof Kozlowski wrote: >>>>> On 08/01/2023 18:09, Trond Myklebust wrote: >>>>>> Hi Krzysztof, >>>>>> >>>>>>> On Jan 8, 2023, at 08:25, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: >>>>>>> >>>>>>> [You don't often get email from krzysztof.kozlowski@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification] >>>>>>> >>>>>>> On 07/01/2023 16:44, Krzysztof Kozlowski wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> Bisect identified commit 7fd461c47c6c ("NFSv4.2: Change the default >>>>>>>> KConfig value for READ_PLUS") as one leading to NULL pointer exception >>>>>>>> when mounting NFS root on NFSv4 client: >>>>>>>> >>>>>>>> [ 25.739003] systemd[1]: Set hostname to <odroidhc1>. >>>>>>>> [ 25.771714] systemd[1]: Failed to bump fs.file-max, ignoring: Invalid >>>>>>>> argument >>>>>>>> [ 26.199478] 8<--- cut here --- >>>>>>>> [ 26.201366] Unable to handle kernel NULL pointer dereference at >>>>>>>> virtual address 00000004 >>>>>>>> ... >>>>>>>> [ 26.555522] mmiocpy from xdr_inline_decode+0xec/0x16c >>>>>>>> [ 26.560628] xdr_inline_decode from nfs4_xdr_dec_read_plus+0x178/0x358 >>>>>>>> [ 26.567130] nfs4_xdr_dec_read_plus from call_decode+0x204/0x304 >>>>>>>> >>>>>>>> Full OOPS attached. Full log available here: >>>>>>>> https://krzk.eu/#/builders/21/builds/3901/steps/15/logs/serial0 >>>>>>>> >>>>>>>> Disabling NFS_V4_2_READ_PLUS fixes the issue, so obviously the commit is >>>>>>>> not the cause, but rather making it default caused the regression. >>>>>>>> >>>>>>>> I did not make the bisect yet which commit introduced it, if every >>>>>>>> config includes NFS_V4_2_READ_PLUS. >>>>>>> >>>>>>> When every kernel is built with NFS_V4_2_READ_PLUS, bisect pointed to: >>>>>>> d3b00a802c84 ("NFS: Replace the READ_PLUS decoding code") >>>>>>> >>>>>>> commit d3b00a802c845a6021148ce2e669b5a0b5729959 >>>>>>> Author: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx> >>>>>>> Date: Thu Jul 21 14:21:34 2022 -0400 >>>>>>> >>>>>>> NFS: Replace the READ_PLUS decoding code >>>>>>> >>>>>>> We now take a 2-step process that allows us to place data and hole >>>>>>> segments directly at their final position in the xdr_stream without >>>>>>> needing to do a bunch of redundant copies to expand holes. Due to the >>>>>>> variable lengths of each segment, the xdr metadata might cross page >>>>>>> boundaries which I account for by setting a small scratch buffer so >>>>>>> xdr_inline_decode() won't fail. >>>>>>> >>>>>>> Signed-off-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx> >>>>>>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> >>>>>>> >>>>>>> With a trace: >>>>>>> [ 25.898462] systemd[1]: Set hostname to <odroidhc1>. >>>>>>> [ 25.933746] systemd[1]: Failed to bump fs.file-max, ignoring: Invalid >>>>>>> argument >>>>>>> [ 25.986237] random: crng init done >>>>>>> [ 26.264564] 8<--- cut here --- >>>>>>> [ 26.266823] Unable to handle kernel NULL pointer dereference at >>>>>>> virtual address 00000fe8 >>>>>>> ... >>>>>>> [ 26.597263] nfs4_xdr_dec_read_plus from call_decode+0x204/0x304 >>>>>>> [ 26.603222] call_decode from __rpc_execute+0xd0/0x890 >>>>>>> [ 26.608328] __rpc_execute from rpc_async_schedule+0x1c/0x34 >>>>>>> [ 26.613960] rpc_async_schedule from process_one_work+0x294/0x790 >>>>>>> [ 26.620030] process_one_work from worker_thread+0x54/0x518 >>>>>>> [ 26.625570] worker_thread from kthread+0xf4/0x128 >>>>>>> [ 26.630336] kthread from ret_from_fork+0x14/0x2c >>>>>>> >>>>>> >>>>>> Is this test being run against a 6.2-rc2 server, or is it an older server platform? We know there were bugs in older server implementations, so the question is whether this might be a problem with handling a bad/corrupt RPC reply from the server, or whether it is happening against code that is supposed to have been fixed? >>>>> >>>>> I would say that buggy server should not cause NULL pointer dereferences >>>>> on the client. Otherwise this is a perfect recipe for a rogue server in >>>>> the network to start crashing clients and running exploits... Imagine a >>>>> compromised machine (through some other means) in a local company >>>>> network running now a server with NFS share "HR salary data" or "HR >>>>> planned layoffs", where unsuspected people in that network access it >>>>> leading to exploit of NFS code on their side... >>>>> >>>>> Server is Raspberry Pi 3 kernel: 5.10.92-2-rpi-legacy-ARCH >>>>> >>>>> Which points that it is not latest stable, so anyway I need to update. >>>> >>>> I updated the kernel to 5.15.84-3-rpi-ARCH which is pretty close to >>>> latest stable and I can reproduce the issue. Therefore: >>>> 1. It is visible on two stable (one new, one old) kernels on the server, >>>> 2. Buggy or rogue server should not cause NULL pointer on remote devices... >>>> >>> >>> The nfsd READ_PLUS code is borked up and until 6.2-rc1. I thought we had a server option to disable that code, but it seems as if that is not the case. >>> Chuck + Anna, can we please send a stable patch to rip out that code altogether from all the older versions? If we want to enable READ_PLUS by default on the client, then we cannot do so if the majority of NFSv4.2 servers out there are running a borked implementation. >>> >>> I do agree that we cannot allow buggy >> >> or malicious, or non-Linux, >> >> >>> servers to cause memory corruption in the client code, so I propose that we revert the Kconfig default setting change again until both the client code and the legacy servers have been fixed. >> >> I stand ready to receive and apply server-side fixes, as you suggested. >> >> However IMO it would be most responsible to go straight for a client code fix. The Kconfig setting is a red herring (as delicious as that might sound). Any thoughts about how difficult that fix might be? > > I'm wondering about how hard the fix might be as well, since it could > be a legitimate bug or some error checking that I've overlooked. I've > gotten as far as installing a 5.15 server in my testing setup, so > we'll see if I'm able to reproduce the crash this morning. All current long-term stable kernels have 72d78717c6d0 ("nfsd: Fixes for nfsd4_encode_read_plus_data()"), and so does v5.10.92. I'm not aware of any other problem with READ_PLUS except for performance. Let me know (or file a bug) if I'm missing something! For the possible performance regression, backporting eeadcb757945 ("NFSD: Simplify READ_PLUS") is the proposed stable fix. * v6.1.3 - eeadcb757945 applies cleanly * v6.0.18 - eeadcb757945 applies with offsets * v5.15.86 - eeadcb757945 needs 1e37d0e5bda4, then applies with offsets * v5.10.162 - would need a hand-written version of eeadcb757945 * v5.4.228 - does not implement READ_PLUS * v4.19.269 - does not implement READ_PLUS * v4.14.302 - does not implement READ_PLUS * v4.9.337 - EOL If someone volunteers to build and test, I would not object to a request to stable@ to apply eeadcb757945 to 5.15, 6.0, and 6.1. -- Chuck Lever
