Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210324130731.1513798-1-arnd@kernel.org> <f3d1c643-56ef-7fe0-52f0-1f030c890a38@rasmusvillemoes.dk>
In-Reply-To: <f3d1c643-56ef-7fe0-52f0-1f030c890a38@rasmusvillemoes.dk>
From:   Arnd Bergmann <arnd@kernel.org>
Date:   Wed, 24 Mar 2021 15:25:47 +0100
Message-ID: <CAK8P3a0YOYSJLTYEEQbm1hvuYy8L-UaGipVbvDuLLSE9zanMGg@mail.gmail.com>
Subject: Re: [PATCH] [v2] hinic: avoid gcc -Wrestrict warning
To:     Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc:     Bin Luo <luobin9@huawei.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Networking <netdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Mar 24, 2021 at 2:29 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 24/03/2021 14.07, Arnd Bergmann wrote:
 >
> >       if (set_settings & HILINK_LINK_SET_SPEED) {
> >               speed_level = hinic_ethtool_to_hw_speed_level(speed);
> >               err = snprintf(set_link_str, SET_LINK_STR_MAX_LEN,
> > -                            "%sspeed %d ", set_link_str, speed);
> > -             if (err <= 0 || err >= SET_LINK_STR_MAX_LEN) {
> > +                            "speed %d ", speed);
> > +             if (err >= SET_LINK_STR_MAX_LEN) {
> >                       netif_err(nic_dev, drv, netdev, "Failed to snprintf link speed, function return(%d) and dest_len(%d)\n",
> >                                 err, SET_LINK_STR_MAX_LEN);
> >                       return -EFAULT;
>
> It's not your invention of course, but this both seems needlessly harsh
> and EFAULT is a weird error to return. It's just a printk() message that
> might be truncated, and now that the format string only has a %d
> specifier, it can actually be verified statically that overflow will
> never happen (though I don't know or think gcc can do that, perhaps
> there's some locale nonsense in the standard that allows using
> utf16-encoded sanskrit runes). So probably that test should just be
> dropped, but that's a separate thing.

I thought about fixing it, but this seemed to be a rabbit hole I didn't
want to get into, as there are other harmless issues in the driver
that could be improved.

I'm fairly sure gcc can indeed warn about the overflow with
-Wformat-truncation, but the warning is disabled at the moment
because it has a ton of false positives:

kernel/workqueue.c: In function 'create_worker':
kernel/workqueue.c:1933:55: error: '%d' directive output may be
truncated writing between 1 and 10 bytes into a region of size between
3 and 13 [-Werror=format-truncation=]
 1933 |                 snprintf(id_buf, sizeof(id_buf), "u%d:%d",
pool->id, id);

        Arnd