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;
Date:   Wed, 24 May 2023 16:06:48 +0200
From:   Jan Kara <jack@suse.cz>
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     Jan Kara <jack@suse.cz>, Jeff Layton <jlayton@kernel.org>,
        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>
Subject: Re: [bug report] fanotify: support reporting non-decodeable file
 handles
Message-ID: <20230524140648.u6pexxspze7pz63z@quack3>
References: <ca02955f-1877-4fde-b453-3c1d22794740@kili.mountain>
 <CAOQ4uxi6ST19WGkZiM=ewoK_9o-7DHvZcAc3v2c5GrqSFf0WDQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAOQ4uxi6ST19WGkZiM=ewoK_9o-7DHvZcAc3v2c5GrqSFf0WDQ@mail.gmail.com>
Precedence: bulk

On Wed 24-05-23 11:38:17, Amir Goldstein wrote:
> On Wed, May 24, 2023 at 9:34 AM 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 = 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 inode *inode,
> >     404                               unsigned int fh_len, unsigned int *hash,
> >     405                               gfp_t gfp)
> >     406 {
> >     407         int dwords, type = 0;
> >     408         char *ext_buf = NULL;
> >     409         void *buf = fh->buf;
> >     410         int err;
> >     411
> >     412         fh->type = FILEID_ROOT;
> >     413         fh->len = 0;
> >     414         fh->flags = 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 could
> >     426          * be zero in that case if encoding fh len failed.
> >     427          */
> >     428         err = -ENOENT;
> >     429         if (fh_len < 4 || WARN_ON_ONCE(fh_len % 4) || fh_len > MAX_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 encode fh */
> >     435                 err = -ENOMEM;
> >     436                 ext_buf = kmalloc(fh_len, gfp);
> >     437                 if (!ext_buf)
> >     438                         goto out_err;
> >     439
> >     440                 *fanotify_fh_ext_buf_ptr(fh) = ext_buf;
> >     441                 buf = ext_buf;
> >     442                 fh->flags |= FANOTIFY_FH_FLAG_EXT_BUF;
> >     443         }
> >     444
> >     445         dwords = fh_len >> 2;
> >     446         type = exportfs_encode_fid(inode, buf, &dwords);
> >     447         err = -EINVAL;
> >     448         if (!type || type == FILEID_INVALID || fh_len != dwords << 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
> filesystem 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.

Yes, I've checked and all ->encode_fh() implementations return
FILEID_INVALID in case of problems (which are basically always only
problems with not enough space in the handle buffer).

> 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 differently:
> nfsd: only type == FILEID_INVALID
> fdinfo: type < 0 || type == FILEID_INVALID
> fanotify: !type || type == FILEID_INVALID
> overlayfs: type < 0 || type == FILEID_INVALID
> name_to_hanle_at: (retval == FILEID_INVALID) || (retval == -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.

Yeah, it's a bit messy. When checking ->encode_fh() implementations I've
also noticed quite some callers use explicit numbers and not FILEID_*
enums. This is not directly related to the problem at hand but I'm voicing
it in case someone looks for an easy cleanup project :)

> 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?

When we have two different error conditions (out of buffer space, encoding
handles unsupported), I agree it makes sense to be able to differentiate
them in exportfs_encode_fh() return value. However then it would make sense
to properly return -ENOSPC instead of FILEID_INVALID (as it is strange to
have two different ways of indicating error) which means touching like 16
different .encode_fh implementations. Not too bad but a bit tedious...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR