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;
Date:   Sun, 10 Jul 2022 01:38:08 -0500
From:   Tyler Hicks <tyhicks@linux.microsoft.com>
To:     Eric Van Hensbergen <ericvh@gmail.com>,
        Latchesar Ionkov <lucho@ionkov.net>,
        Dominique Martinet <asmadeus@codewreck.org>,
        Christian Schoenebeck <linux_oss@crudebyte.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        v9fs-developer@lists.sourceforge.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net/9p: Initialize the iounit field during fid creation
Message-ID: <20220710063808.GB272934@sequoia>
References: <20220709200005.681861-1-tyhicks@linux.microsoft.com>
 <20220710062557.GA272934@sequoia>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220710062557.GA272934@sequoia>
Precedence: bulk

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