linux-kernel - Re: [PATCH] net: mlx5: remove pointless memcpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <99c7fe90-824e-b32a-295b-b7cac3370801@mellanox.com>
Date:   Fri, 2 Feb 2018 20:38:46 -0800
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     Arnd Bergmann <arnd@...db.de>, Jason Gunthorpe <jgg@...lanox.com>
Cc:     Matan Barak <matanb@...lanox.com>,
        Leon Romanovsky <leonro@...lanox.com>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Networking <netdev@...r.kernel.org>,
        linux-rdma <linux-rdma@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: mlx5: remove pointless memcpy



On 02/02/2018 12:26 PM, Arnd Bergmann wrote:
> On Fri, Feb 2, 2018 at 8:06 PM, Jason Gunthorpe <jgg@...lanox.com> wrote:
>> On Fri, Feb 02, 2018 at 04:46:30PM +0100, Arnd Bergmann wrote:
>>> gcc-8 notices that the memcpy in mlx5_core_query_xsrq() makes no
>>> sense because the source and destination variables are identical:
>>>
>>> drivers/net/ethernet/mellanox/mlx5/core/transobj.c: In function 'mlx5_core_query_xsrq':
>>> drivers/net/ethernet/mellanox/mlx5/core/transobj.c:347:3: error: 'memcpy' source argument is the same as destination [-Werror=restrict]
>>>
>>> Either one of the pointers should be something else, or the code is
>>> completely bogus. Removing the memcpy() won't change the behavior
>>> but gets rid of the warning.
>>>
>>> Fixes: 01949d0109ee ("net/mlx5_core: Enable XRCs and SRQs when using ISSI > 0")
>>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
>>> Please review carefully, I have no idea what the author actually
>>> intended here.
>>
>> I think they intended to adjust the command return between
>> mlx5_ifc_query_srq_out_bits and mlx5_ifc_query_xrc_srq_out_bits?
>>
>>> index 9e38343a951f..75450f7d53bf 100644
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/transobj.c
>>> @@ -332,20 +332,12 @@ int mlx5_core_destroy_xsrq(struct mlx5_core_dev *dev, u32 xsrqn)
>>>  int mlx5_core_query_xsrq(struct mlx5_core_dev *dev, u32 xsrqn, u32 *out)
>>>  {
>>>       u32 in[MLX5_ST_SZ_DW(query_xrc_srq_in)] = {0};
>>> -     void *srqc;
>>> -     void *xrc_srqc;
>>>       int err;
>>>
>>>       MLX5_SET(query_xrc_srq_in, in, opcode,   MLX5_CMD_OP_QUERY_XRC_SRQ);
>>>       MLX5_SET(query_xrc_srq_in, in, xrc_srqn, xsrqn);
>>>       err = mlx5_cmd_exec(dev, in, sizeof(in), out,
>>>                           MLX5_ST_SZ_BYTES(query_xrc_srq_out));
>>> -     if (!err) {
>>> -             xrc_srqc = MLX5_ADDR_OF(query_xrc_srq_out, out,
>>> -                                     xrc_srq_context_entry);
>>> -             srqc = MLX5_ADDR_OF(query_srq_out, out, srq_context_entry);
>>> -             memcpy(srqc, xrc_srqc, MLX5_ST_SZ_BYTES(srqc));
>>> -     }

OMG!

>>
>> Probably should add a
>>
>> BUILD_BUG_ON(MLX5_BYTE_OFF(query_xrc_srq_out, xrc_srq_context_entry) == MLX5_BYTE_OFF(query_srq_out, srq_context_entry));
>>
>> Just for clarity that the SRQ and XRC_SRQ are being used interchangeably.
>>
>> and the 'err' variable can be eliminated.
>>
>> Curious though that I can't find a call site for it, and removing the
>> prototype doesn't break the build.. Seems like dead code.
> 
> I checked the git history and don't see any user ever added after the function
> first showed up in the kernel, same for a couple of other functions from
> commit 01949d0109ee ("net/mlx5_core: Enable XRCs and SRQs when
> using ISSI > 0").
> 
> Can you come up with a proper patch for this isse, either removing the
> dead code, or fixing it appropriately? You clearly understand what this
> file is about, and I don't ;-)

Simply this is just pointless dead code, will remove it, there is no point of trying to
figure out what the author was thinking the day he wrote that patch :)

Thank you Arnd for spotting this.

> 
>       Arnd
>