Received-SPF: pass (google.com: domain of linux-kernel+bounces-14570-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <140431.1704208899@warthog.procyon.org.uk>
In-Reply-To: <140431.1704208899@warthog.procyon.org.uk>
From: Marc Dionne <marc.dionne@auristor.com>
Date: Tue, 2 Jan 2024 11:41:59 -0400
Message-ID: <CAB9dFdvGN=fkuFK++V6ovvCXCQBecQ+JVCh=a8tLzmgVqkk==w@mail.gmail.com>
Subject: Re: [PATCH] afs: Fix error handling with lookup via FS.InlineBulkStatus
To: David Howells <dhowells@redhat.com>
Cc: Jeffrey Altman <jaltman@auristor.com>, linux-afs@lists.infradead.org, 
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 2, 2024 at 11:21=E2=80=AFAM David Howells <dhowells@redhat.com>=
 wrote:
>
> When afs does a lookup, it tries to use FS.InlineBulkStatus to preemptive=
ly
> look up a bunch of files in the parent directory and cache this locally, =
on
> the basis that we might want to look at them too (for example if someone
> does an ls on a directory, they may want want to then stat every file
> listed).
>
> FS.InlineBulkStatus can be considered a compound op with the normal abort
> code applying to the compound as a whole.  Each status fetch within the
> compound is then given its own individual abort code - but assuming no
> error that prevents the bulk fetch from returning the compound result wil=
l
> be 0, even if all the constituent status fetches failed.
>
> At the conclusion of afs_do_lookup(), we should use the abort code from t=
he
> appropriate status to determine the error to return, if any - but instead
> it is assumed that we were successful if the op as a whole succeeded and =
we
> return an incompletely initialised inode, resulting in ENOENT, no matter
> the actual reason.  In the particular instance reported, a vnode with no
> permission granted to be accessed is being given a UAEACCES abort code
> which should be reported as EACCES, but is instead being reported as
> ENOENT.
>
> Fix this by abandoning the inode (which will be cleaned up with the op) i=
f
> file[1] has an abort code indicated and turn that abort code into an erro=
r
> instead.
>
> Whilst we're at it, add a tracepoint so that the abort codes of the
> individual subrequests of FS.InlineBulkStatus can be logged.  At the mome=
nt
> only the container abort code can be 0.
>
> Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" con=
cept")
> Reported-by: Jeffrey Altman <jaltman@auristor.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> ---
>  fs/afs/dir.c               |   12 +++++++++---
>  include/trace/events/afs.h |   25 +++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index c14533ef108f..ae563d2a914e 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -708,6 +708,8 @@ static void afs_do_lookup_success(struct afs_operatio=
n *op)
>                         break;
>                 }
>
> +               if (vp->scb.status.abort_code)
> +                       trace_afs_bulkstat_error(op, &vp->fid, i, vp->scb=
.status.abort_code);
>                 if (!vp->scb.have_status && !vp->scb.have_error)
>                         continue;
>
> @@ -897,12 +899,16 @@ static struct inode *afs_do_lookup(struct inode *di=
r, struct dentry *dentry,
>                 afs_begin_vnode_operation(op);
>                 afs_wait_for_operation(op);
>         }
> -       inode =3D ERR_PTR(afs_op_error(op));
>
>  out_op:
>         if (!afs_op_error(op)) {
> -               inode =3D &op->file[1].vnode->netfs.inode;
> -               op->file[1].vnode =3D NULL;
> +               if (op->file[1].scb.status.abort_code) {
> +                       afs_op_accumulate_error(op, -ECONNABORTED,
> +                                               op->file[1].scb.status.ab=
ort_code);
> +               } else {
> +                       inode =3D &op->file[1].vnode->netfs.inode;
> +                       op->file[1].vnode =3D NULL;
> +               }
>         }
>
>         if (op->file[0].scb.have_status)
> diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h
> index 5194b7e6dc8d..ce865ea678d3 100644
> --- a/include/trace/events/afs.h
> +++ b/include/trace/events/afs.h
> @@ -1102,6 +1102,31 @@ TRACE_EVENT(afs_file_error,
>                       __print_symbolic(__entry->where, afs_file_errors))
>             );
>
> +TRACE_EVENT(afs_bulkstat_error,
> +           TP_PROTO(struct afs_operation *op, struct afs_fid *fid, unsig=
ned int index, s32 abort),
> +
> +           TP_ARGS(op, fid, index, abort),
> +
> +           TP_STRUCT__entry(
> +                   __field_struct(struct afs_fid,      fid)
> +                   __field(unsigned int,               op)
> +                   __field(unsigned int,               index)
> +                   __field(s32,                        abort)
> +                            ),
> +
> +           TP_fast_assign(
> +                   __entry->op =3D op->debug_id;
> +                   __entry->fid =3D *fid;
> +                   __entry->index =3D index;
> +                   __entry->abort =3D abort;
> +                          ),
> +
> +           TP_printk("OP=3D%08x[%02x] %llx:%llx:%x a=3D%d",
> +                     __entry->op, __entry->index,
> +                     __entry->fid.vid, __entry->fid.vnode, __entry->fid.=
unique,
> +                     __entry->abort)
> +           );
> +
>  TRACE_EVENT(afs_cm_no_server,
>             TP_PROTO(struct afs_call *call, struct sockaddr_rxrpc *srx),

Reviewed-by: Marc Dionne <marc.dionne@auristor.com>

Marc