netdev - Re: [PATCH] net: usb: pegasus: ignore the return value from set

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: <20210816070640.2a7a6f5d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 16 Aug 2021 07:06:40 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Petko Manolov <petko.manolov@...sulko.com>
Cc:     netdev@...r.kernel.org, paskripkin@...il.com,
        stable@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH] net: usb: pegasus: ignore the return value from
 set_registers();

On Sun, 15 Aug 2021 11:54:55 +0300 Petko Manolov wrote:
> > > @@ -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 ||  
> > 
> > This one is not added by the recent changes as I initially thought, 
> > the driver has always checked this return value. The recent changes 
> > did this:
> > 
> >         ret = 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 ||
> >             usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
> >                 u16 auxmode;
> > -               read_mii_word(pegasus, 0, 0x1b, &auxmode);
> > +               ret = read_mii_word(pegasus, 0, 0x1b, &auxmode);
> > +               if (ret < 0)
> > +                       goto fail;
> >                 auxmode |= 4;
> >                 write_mii_word(pegasus, 0, 0x1b, &auxmode);
> >         }
> >  
> > +       return 0;
> > +fail:
> > +       netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
> >         return ret;
> > }
> > 
> > now the return value of set_registeres() is ignored. 
> > 
> > Seems like  a better fix would be to bring back the error checking, 
> > why not?  
> 
> Mostly because for this particular adapter checking the read failure makes much
> more sense than write failure.

This is not an either-or choice.

> Checking the return value of set_register(s) is often usless because device's
> default register values are sane enough to get a working ethernet adapter even
> without much prodding.  There are exceptions, though, one of them being
> set_ethernet_addr().
> 
> You could read the discussing in the netdev ML, but the essence of it is that
> set_ethernet_addr() should not give up if set_register(s) fail.  Instead, the
> driver should assign a valid, even if random, MAC address.
> 
> It is much the same situation with enable_net_traffic() - it should continue
> regardless.  There are two options to resolve this: a) remove the error check
> altogether; b) do the check and print a debug message.  I prefer a), but i am
> also not strongly opposed to b).  Comments?

c) keep propagating the error like the driver used to.

I don't understand why that's not the most obvious option.

The driver used to propagate the errors from the set_registers() call
in enable_net_traffic() since the beginning of the git era. This is 
_not_ one of the error checking that you recently added.