Re: [GIT PULL] Please pull NFS changes for Linux 5.3

[Linus Torvalds]

On Thu, Jul 18, 2019 at 1:25 PM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
>   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git tags/nfs-for-5.3-1

This got a conflict with the debugfs "don't behave differently on
failures" changes in net/sunrpc/debugfs.c.

See commit 0a0762c6c604 ("sunrpc: no need to check return value of
debugfs_create functions") by Greg, but I suspect you were already
aware of this.

I did a hack-and-slash "remove the error handling", and the end result
looks sane. Except I left the "if the snprintf overflows" error
handling in place, even if nothing then cares about the returned
error.

I think my merge resolution makes sense, but I thought I'd mention it
in case you had something else in mind. Honestly, the snprintf()
checks in do_xprt_debugfs() look kind o fpointless, but the comment is
also wrong:

        char link[9]; /* enough for 8 hex digits + NULL */

that comment was copied from the "name[]" array in
rpc_clnt_debugfs_register(), but it's bogus, since you actually use

                len = snprintf(link, sizeof(link), "xprt%d", *nump);

on the thing.

And you know what? If you have so many links that "xprt%d" doesn't fit
in 8 chars plus NUL, maybe you don't really care?

But it's also worth noting that the whole snprintf() overflow check is
*wrong* to begin with. When you do

                if (len > sizeof(link))
                        return -1;

you're testing the wrong thing entirely. The returned "len" is the
length that would have been printed _without_ the ending NUL
character, so you actually had a truncation even if it returns
"sizeof(link)" - because then the NUL character was written instead of
the last character.

So the overflow test *should* have been

                if (len >= sizeof(link))
                        return -1;

but I suspect the correct thing to do is to just say "we don't care"
and remove that error check entirely.  Same goes for the other case
("len > sizeof(name)").

At some point error handling doesn't actually add value, as long as
the error itself isn't fatal. And when the error handling itself is
wrong, it's doubly suspect.

But as mentioned, I did *not* remove this part of the error handling.
I only removed the debugfs parts. The error handling may be wrong, but
it is what it is, and it doesn't really matter.

                  Linus