Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <ca02955f-1877-4fde-b453-3c1d22794740@kili.mountain>
In-Reply-To: <ca02955f-1877-4fde-b453-3c1d22794740@kili.mountain>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Wed, 24 May 2023 11:38:17 +0300
Message-ID: <CAOQ4uxi6ST19WGkZiM=ewoK_9o-7DHvZcAc3v2c5GrqSFf0WDQ@mail.gmail.com>
Subject: Re: [bug report] fanotify: support reporting non-decodeable file handles
To:     Jan Kara <jack@suse.cz>, Jeff Layton <jlayton@kernel.org>
Cc:     linux-fsdevel@vger.kernel.org,
        Dan Carpenter <dan.carpenter@linaro.org>,
        Chuck Lever <cel@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 24, 2023 at 9:34=E2=80=AFAM Dan Carpenter <dan.carpenter@linaro=
.org> wrote:
>
> Hello Amir Goldstein,
>
> The patch 7ba39960c7f3: "fanotify: support reporting non-decodeable
> file handles" from May 2, 2023, leads to the following Smatch static
> checker warning:
>
>         fs/notify/fanotify/fanotify.c:451 fanotify_encode_fh()
>         warn: assigning signed to unsigned: 'fh->type =3D type' 's32min-(=
-1),1-254,256-s32max'
>
> (unpublished garbage Smatch check).
>
> fs/notify/fanotify/fanotify.c
>     403 static int fanotify_encode_fh(struct fanotify_fh *fh, struct inod=
e *inode,
>     404                               unsigned int fh_len, unsigned int *=
hash,
>     405                               gfp_t gfp)
>     406 {
>     407         int dwords, type =3D 0;
>     408         char *ext_buf =3D NULL;
>     409         void *buf =3D fh->buf;
>     410         int err;
>     411
>     412         fh->type =3D FILEID_ROOT;
>     413         fh->len =3D 0;
>     414         fh->flags =3D 0;
>     415
>     416         /*
>     417          * Invalid FHs are used by FAN_FS_ERROR for errors not
>     418          * linked to any inode. The f_handle won't be reported
>     419          * back to userspace.
>     420          */
>     421         if (!inode)
>     422                 goto out;
>     423
>     424         /*
>     425          * !gpf means preallocated variable size fh, but fh_len c=
ould
>     426          * be zero in that case if encoding fh len failed.
>     427          */
>     428         err =3D -ENOENT;
>     429         if (fh_len < 4 || WARN_ON_ONCE(fh_len % 4) || fh_len > MA=
X_HANDLE_SZ)
>     430                 goto out_err;
>     431
>     432         /* No external buffer in a variable size allocated fh */
>     433         if (gfp && fh_len > FANOTIFY_INLINE_FH_LEN) {
>     434                 /* Treat failure to allocate fh as failure to enc=
ode fh */
>     435                 err =3D -ENOMEM;
>     436                 ext_buf =3D kmalloc(fh_len, gfp);
>     437                 if (!ext_buf)
>     438                         goto out_err;
>     439
>     440                 *fanotify_fh_ext_buf_ptr(fh) =3D ext_buf;
>     441                 buf =3D ext_buf;
>     442                 fh->flags |=3D FANOTIFY_FH_FLAG_EXT_BUF;
>     443         }
>     444
>     445         dwords =3D fh_len >> 2;
>     446         type =3D exportfs_encode_fid(inode, buf, &dwords);
>     447         err =3D -EINVAL;
>     448         if (!type || type =3D=3D FILEID_INVALID || fh_len !=3D dw=
ords << 2)
>
> exportfs_encode_fid() can return negative errors.  Do we need to check
> if (!type etc?

Well, it is true that exportfs_encode_fid() can return a negative value
in principle, as did exportfs_encode_fh() before it, if there was a filesys=
tem
implementation of ->encode_fh() that returned a negative value.
AFAIK, there currently is no such implementation in-tree, otherwise current
upstream code would have been buggy.

Patch 2/4 adds a new possible -EOPNOTSUPP return value from
exportfs_encode_inode_fh() and even goes further to add a kerndoc:
 * Returns an enum fid_type or a negative errno.
But this new return value is not possible from exportfs_encode_fid()
that is used here and in {fa,i}notify_fdinfo().

All the rest of the callers (nfsd, overlayfs, name_to_hanle_at) already
check this same EOPNOTSUPP condition before calling, but there is
no guarantee that this will not change in the future.

All the callers mentioned above check the unexpected return value different=
ly:
nfsd: only type =3D=3D FILEID_INVALID
fdinfo: type < 0 || type =3D=3D FILEID_INVALID
fanotify: !type || type =3D=3D FILEID_INVALID
overlayfs: type < 0 || type =3D=3D FILEID_INVALID
name_to_hanle_at: (retval =3D=3D FILEID_INVALID) || (retval =3D=3D -ENOSPC)=
)
                /* As per old exportfs_encode_fh documentation
                 * we could return ENOSPC to indicate overflow
                 * But file system returned 255 always. So handle
                 * both the values
                 */

So he have a bit of a mess.
How should we clean it up?

Option #1: Change encode_fh to return unsigned and replace that new
                  EOPNOTSUPP with FILEID_INVALID
Option #2: change all callers to check negative return value

I am in favor of option #2.
Shall I send a patch?

Thanks,
Amir.