Received-SPF: pass (google.com: domain of linux-nfs+bounces-2385-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Trond Myklebust" <trondmy@hammerspace.com>
Cc: "anna@kernel.org" <anna@kernel.org>,
 "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject:
 Re: Question about possible use-after-free in nfs_direct_write_reschedule()
In-reply-to: <02300d56202423fc7277b0ee923b40f00d4903db.camel@hammerspace.com>
References: <171080858885.13576.7878757943353384571@noble.neil.brown.name>,
 <02300d56202423fc7277b0ee923b40f00d4903db.camel@hammerspace.com>
Date: Tue, 19 Mar 2024 16:12:29 +1100
Message-id: <171082514968.13576.13734805489700296059@noble.neil.brown.name>

On Tue, 19 Mar 2024, Trond Myklebust wrote:
> On Tue, 2024-03-19 at 11:36 +1100, NeilBrown wrote:
> > 
> > I've been reviewing some nfs/direct.c patches for possible backport
> > to
> > one of our older enterprise kernels and I've found something that
> > looks
> > wrong.
> > 
> > It isn't clear to me how to exercise the code so I haven't be able to
> > trigger a problem.  I'm hoping that someone could either explain when
> > this code runs, or confirm if the code is correct or not.
> > 
> > Commit 954998b60caa ("NFS: Fix error handling for O_DIRECT write
> > scheduling")
> > 
> > adds an extra call to nfs_release_request() but I cannot find any
> > place
> > that an extra reference is taken.
> > 
> > The code currently reads:
> > 
> > 	while (!list_empty(&reqs)) {
> > 		req = nfs_list_entry(reqs.next);
> > 		nfs_list_remove_request(req);
> > 		nfs_unlock_and_release_request(req);
> > 		if (desc.pg_error == -EAGAIN) {
> > 			nfs_mark_request_commit(req, NULL, &cinfo,
> > 0);
> > 		} else {
> > 			spin_lock(&dreq->lock);
> > 			nfs_direct_truncate_request(dreq, req);
> > 			spin_unlock(&dreq->lock);
> > 			nfs_release_request(req);
> > 		}
> > 	}
> > 
> > after the nfs_unlock_and_release_request() call I would expect that
> > the
> > request could be freed, so that nfs_mark_request_commit() or the
> > nfs_release_request() could cause a problem.
> > 
> > Superficially it looks like the call should be simply
> > nfs_unlock_request().  This would follow the
> > list_remove;unlock;mark_commit pattern also found in
> > nfs_direct_write_reschedule_io().
> > 
> > Do we need:
> > --- a/fs/nfs/direct.c
> > +++ b/fs/nfs/direct.c
> > @@ -581,7 +581,7 @@ static void nfs_direct_write_reschedule(struct
> > nfs_direct_req *dreq)
> >  	while (!list_empty(&reqs)) {
> >  		req = nfs_list_entry(reqs.next);
> >  		nfs_list_remove_request(req);
> > -		nfs_unlock_and_release_request(req);
> > +		nfs_unlock_request(req);
> >  		if (desc.pg_error == -EAGAIN) {
> >  			nfs_mark_request_commit(req, NULL, &cinfo,
> > 0);
> >  		} else {
> 
> See the full code that was changed:
>  
> -       list_for_each_entry_safe(req, tmp, &reqs, wb_list) {
> +       while (!list_empty(&reqs)) {
> +               req = nfs_list_entry(reqs.next);
>                 /* Bump the transmission count */
>                 req->wb_nio++;
>                 if (!nfs_pageio_add_request(&desc, req)) {
> -                       nfs_list_move_request(req, &failed);
>                         spin_lock(&cinfo.inode->i_lock);
> -                       dreq->flags = 0;
> -                       if (desc.pg_error < 0)
> +                       if (dreq->error < 0) {
> +                               desc.pg_error = dreq->error;
> +                       } else if (desc.pg_error != -EAGAIN) {
> +                               dreq->flags = 0;
> +                               if (!desc.pg_error)
> +                                       desc.pg_error = -EIO;
>                                 dreq->error = desc.pg_error;
> -                       else
> -                               dreq->error = -EIO;
> +                       } else
> +                               dreq->flags =
> NFS_ODIRECT_RESCHED_WRITES;
>                         spin_unlock(&cinfo.inode->i_lock);
> +                       break;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Prior to this patch, we did not break out of the loop until the entire
> "reqs" list hand been handled.
> 
>                 }
>                 nfs_release_request(req);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Prior to this patch, every request on the "reqs" list was released,
> whether or not they were being moved to the "failed" list.
>         }
>         nfs_pageio_complete(&desc);
>  
> -       while (!list_empty(&failed)) {
> -               req = nfs_list_entry(failed.next);
> +       while (!list_empty(&reqs)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Prior to this patch, every request that was being handled here had
> already seen a call to nfs_release_request() because we had already
> gone through the entire list of "reqs".
> With this patch applied, we're now handling all the requests that are
> left on "reqs", and that have not been released.

Ahhh - I get it now - thanks a lot for the explanation.

NeilBrown


> 
> +               req = nfs_list_entry(reqs.next);
>                 nfs_list_remove_request(req);
>                 nfs_unlock_and_release_request(req);
> +               if (desc.pg_error == -EAGAIN)
> +                       nfs_mark_request_commit(req, NULL, &cinfo, 0);
> +               else
> +                       nfs_release_request(req);
>         }
>  
> 
> > 
> > ??
> > 
> > Thanks,
> > NeilBrown
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
> 
>