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

Date:   Tue, 17 Aug 2021 17:11:15 +0300
From:   Petko Manolov <petko.manolov@...sulko.com>
To:     Jakub Kicinski <kuba@...nel.org>
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 21-08-16 13:18:15, Jakub Kicinski wrote:
> On Mon, 16 Aug 2021 22:14:47 +0300 Petko Manolov wrote:
> > On 21-08-16 07:06:40, Jakub Kicinski wrote:
> > > On Sun, 15 Aug 2021 11:54:55 +0300 Petko Manolov wrote:  
> > > > 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.  
> > 
> > If you carefully read the code, which dates back to at least 2005, you'll see
> > that on line 436 (v5.14-rc6) 'ret' is assigned with the return value of
> > set_registers(), but 'ret' is never evaluated and thus not acted upon.
> 
> It's no longer evaluated because of your commit 8a160e2e9aeb ("net:
> usb: pegasus: Check the return value of get_geristers() and friends;")
> IOW v5.14-rc6 has your recent patch. Which I quoted earlier in this
> thread. That commit was on Aug 3 2021. The error checking (now
> accidentally removed) was introduced somewhere in 2.6.x days.
> 
> If you disagree with that please show me the code you're referring to,
> because I just don't see it.
> 
> > > I don't understand why that's not the most obvious option.  
> > 
> > Which part of "this is not a fatal error" you did not understand?
> 
> That's not the point. The error checking was removed accidentally, it should
> be brought back in net to avoid introducing regressions.

Not introducing regressions is the argument that sold me on.  If i don't get too
lazy i may further change this part of the code, but that's in the future.  I've
sent a patch that restores the original behavior in addition of a few small
tweaks.


		Petko