linux-kernel - Re: drivers/net/usb/pegasus.c:461:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]

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]

Message-ID: <YRF1t5kx6hTrv5LC@carbon>
Date:   Mon, 9 Aug 2021 21:36:39 +0300
From:   Petko Manolov <petkan@...leusys.com>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     kernel test robot <rong.a.chen@...el.com>,
        clang-built-linux@...glegroups.com, kbuild-all@...ts.01.org,
        linux-kernel@...r.kernel.org
Subject: Re: drivers/net/usb/pegasus.c:461:2: warning: Value stored to 'ret'
 is never read [clang-analyzer-deadcode.DeadStores]

On 21-08-09 14:06:11, Pavel Skripkin wrote:
> On 8/9/21 1:37 PM, kernel test robot wrote:
> > 
> > tree:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   85a90500f9a1717c4e142ce92e6c1cb1a339ec78
> > commit: 8a160e2e9aeb8318159b48701ad8a6e22274372d net: usb: pegasus:
> > Check the return value of get_geristers() and friends;
> > date:   4 days ago
> > :::::: branch date: 8 hours ago
> > :::::: commit date: 4 days ago
> > config: x86_64-randconfig-c001-20210808 (attached as .config)
> > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
> > 41a6b50c25961addc04438b567ee1f4ef9e40f98)
> > reproduce (this is a W=1 build):
> >           wget
> > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> > -O ~/bin/make.cross
> >           chmod +x ~/bin/make.cross
> >           # install x86_64 cross compiling tool for clang build
> >           # apt-get install binutils-x86-64-linux-gnu
> >           #
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8a160e2e9aeb8318159b48701ad8a6e22274372d
> >           git remote add linus
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> >           git fetch --no-tags linus master
> >           git checkout 8a160e2e9aeb8318159b48701ad8a6e22274372d
> >           # save the attached .config to linux build tree
> >           COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
> > ARCH=x86_64 clang-analyzer
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@...el.com>
> 
> Hi, @Petko!
> 
> For you not to scan all these warnings:
> 
> > > > drivers/net/usb/pegasus.c:461:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
> >              ret = set_registers(pegasus, EthCtrl0, 3, data);
> >              ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >      drivers/net/usb/pegasus.c:461:2: note: Value stored to 'ret' is
> > never read
> >              ret = set_registers(pegasus, EthCtrl0, 3, data);
> >              ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This is the real bug, I think. Can be fixed like this:
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 22353bab76c8..f2b8891c7b36 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -459,6 +459,8 @@ static int enable_net_traffic(struct net_device *dev,
> struct usb_device *usb)
> 
>  	memcpy(pegasus->eth_regs, data, sizeof(data));
>  	ret = set_registers(pegasus, EthCtrl0, 3, data);
> +	if (ret < 0)
> +		goto fail;
> 
>  	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
>  	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
> 
> 
> It was caused by our last refactoring: enable_net_traffic() now returns 0 on
> success and this ret is never checked.

I'd rather remove the 'ret = ' part and leave set_registers() alone.  If this
particular write operation fail, it doesn't mean the adapter won't work at all.
Perhaps it won't be the most optimal mode, but it will work.  There are some
legal checks after set_registers() that also make sense to pass.  So the patch i
suggest looks like:


diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 652e9fcf0b77..49cfc720d78f 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -433,7 +433,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
        data[2] = loopback ? 0x09 : 0x01;

        memcpy(pegasus->eth_regs, data, sizeof(data));
-       ret = set_registers(pegasus, EthCtrl0, 3, data);
+       set_registers(pegasus, EthCtrl0, 3, data);

        if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
            usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||


cheers,
Petko