Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <333e896cf5bcadd8547fbe4a06388dd3104ff910.camel@hammerspace.com>
In-Reply-To: <333e896cf5bcadd8547fbe4a06388dd3104ff910.camel@hammerspace.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 18 Jul 2019 14:47:56 -0700
Message-ID: <CAHk-=wiiLJ3r2t02iqCtMxTufTCpKrPBmQ_L7ePZ4f-MwJ8o6A@mail.gmail.com>
Subject: Re: [GIT PULL] Please pull NFS changes for Linux 5.3
To:     Trond Myklebust <trondmy@hammerspace.com>
Cc:     "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 18, 2019 at 1:25 PM Trond Myklebust <trondmy@hammerspace.com> 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