Messages in this thread | | | Date | Sun, 10 Jul 2022 01:38:08 -0500 | From | Tyler Hicks <> | Subject | Re: [PATCH] net/9p: Initialize the iounit field during fid creation |
| |
On 2022-07-10 01:26:13, Tyler Hicks wrote: > On 2022-07-09 15:00:05, Tyler Hicks wrote: > > Ensure that the fid's iounit field is set to zero when a new fid is > > created. Certain 9P operations, such as OPEN and CREATE, allow the > > server to reply with an iounit size which the client code assigns to the > > fid struct shortly after the fid is created in p9_fid_create(). Other > > operations that follow a call to p9_fid_create(), such as an XATTRWALK, > > don't include an iounit value in the reply message from the server. In > > the latter case, the iounit field remained uninitialized. Depending on > > allocation patterns, the iounit value could have been something > > reasonable that was carried over from previously freed fids or, in the > > worst case, could have been arbitrary values from non-fid related usages > > of the memory location. > > > > The bug was detected in the Windows Subsystem for Linux 2 (WSL2) kernel > > after the uninitialized iounit field resulted in the typical sequence of > > two getxattr(2) syscalls, one to get the size of an xattr and another > > after allocating a sufficiently sized buffer to fit the xattr value, to > > hit an unexpected ERANGE error in the second call to getxattr(2). An > > uninitialized iounit field would sometimes force rsize to be smaller > > than the xattr value size in p9_client_read_once() and the 9P server in > > WSL refused to chunk up the READ on the attr_fid and, instead, returned > > ERANGE to the client. The virtfs server in QEMU seems happy to chunk up > > the READ and this problem goes undetected there. However, there are > > likely other non-xattr implications of this bug that could cause > > inefficient communication between the client and server.
^ I think this last sentence can be removed. I now believe that this only affects xattr get/set operations because nothing else calling the functions that honor iounit is getting the fid directly from a call to p9_fid_create().
> >
Please add the following tag:
Fixes: ebf46264a004 ("fs/9p: Add support user. xattr")
I'm happy to do both of these things in a v2 if any changes/improvements are requested. Thanks!
Tyler
> > Cc: stable@vger.kernel.org > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > --- > > > > Note that I haven't had a chance to identify when this bug was > > introduced so I don't yet have a proper Fixes tag. The history looked a > > little tricky to me but I'll have another look in the coming days. We > > started hitting this bug after trying to move from linux-5.10.y to > > linux-5.15.y but I didn't see any obvious changes between those two > > series. I'm not confident of this theory but perhaps the fid refcounting > > changes impacted the fid allocation patterns enough to uncover the > > latent bug? > > From reading the source, I believe that this first showed up in commit > ebf46264a004 ("fs/9p: Add support user. xattr") which landed in v2.6.36. > Before that commit, p9_client_read(), p9_client_write(), and > p9_client_readdir() were always passed a fid that came from a file's > private_data and went through the open/create functions that initialized > iounit. That commit was the first that passed a fid directly from > p9_fid_create() to p9_client_read(). > > Tyler
| |