netdev - Re: [PATCH net] net: hns3: fix a wrong reset interrupt status mask

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Nov 2019 15:36:03 -0800 (PST)
From:   David Miller <davem@...emloft.net>
To:     tanhuazhong@...wei.com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        salil.mehta@...wei.com, yisen.zhuang@...wei.com,
        linuxarm@...wei.com, jakub.kicinski@...ronome.com
Subject: Re: [PATCH net] net: hns3: fix a wrong reset interrupt status mask

From: Huazhong Tan <tanhuazhong@...wei.com>
Date: Tue, 19 Nov 2019 10:31:48 +0800

> According to hardware user manual, bits5~7 in register
> HCLGE_MISC_VECTOR_INT_STS means reset interrupts status,
> but HCLGE_RESET_INT_M is defined as bits0~2 now. So it
> will make hclge_reset_err_handle() read the wrong reset
> interrupt status.
> 
> This patch fixes it and prints out the register value.
> 
> Fixes: 2336f19d7892 ("net: hns3: check reset interrupt status when reset fails")
> Signed-off-by: Huazhong Tan <tanhuazhong@...wei.com>

Fix exactly _one_ thing or else you make your patch hard to review.

The bug is that the bits are wrong, just fix the bits!

>  
> +	u32 msix_sts_reg;
> +
> +	msix_sts_reg = hclge_read_dev(&hdev->hw, HCLGE_MISC_VECTOR_INT_STS);
> +
>  	if (hdev->reset_pending) {

Now you are reading a register, and potentially clearing status bits and
causing other side effects, that would not happen in this code path
where hdev->reset_pending is true.

Don't do stuff like this!

If you want to add code to print out the register value, that is a
separate patch, for net-next, and it must be done properly.  In that
you should only read the register in the same code paths you do
previously.   Otherwise you must _clearly_ explain why reading the
register value in new code paths is OK, and the side effects will
not potentially cause problems for the pending reset operation.  It
is still going to be a net-next improvement only.

Thank you.