netdev - Re: [PATCH] forcedeth: mac address fix

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:	Wed, 2 Apr 2008 02:24:59 +0200
From:	Björn Steinbrink <B.Steinbrink@....de>
To:	Ayaz Abdulla <aabdulla@...dia.com>
Cc:	Jeff Garzik <jgarzik@...ox.com>,
	Manfred Spraul <manfred@...orfullife.com>,
	Andrew Morton <akpm@...l.org>, nedev <netdev@...r.kernel.org>
Subject: Re: [PATCH] forcedeth: mac address fix

On 2008.03.31 19:24:24 -0500, Ayaz Abdulla wrote:
>
>
> Björn Steinbrink wrote:
>> On 2008.03.31 16:10:34 -0500, Ayaz Abdulla wrote:
>>
>>> This critical patch fixes a mac address issue recently introduced. If 
>>>  
>>
>>
>> Does "recently" mean my commit 2e3884b5b16795c03a7bf295797c1b2402885b88?
>> If so, I like to be told directly when I break stuff ;-)
>>
> Thats why I cc'd you. :)

OK, it's just that "recently broken" can mean about anything ;-) So I
would have welcomed a "broken by xxx" in the commit message, or a small
note below the --- line. :-)

>>> the device's mac address was in correct order and the flag   
>>> NVREG_TRANSMITPOLL_MAC_ADDR_REV was set, during nv_remove the flag 
>>> would  get cleared. During next load, the mac address would get 
>>> reversed  because the flag is missing.
>>
>>
>> Hm, but nv_remove also writes back the reversed mac address. I don't see
>> how a plain remove/probe cycle would mess things up.
>>
>>
>
> For example, NVREG_TRANSMITPOLL_MAC_ADDR_REV is set. That would mean  
> that orig_mac will be stored with correct address. Then you call  
> nv_remove (via ifdown) which set orig_mac back into the register and  
> will clear the flag. On the next nv_probe (via ifup), you would perform  
> the logic to reverse the mac address. But it is still in correct order.

OK, that's the case when we had two consecutive nv_probe calls, without
a call to nv_remove in between, right? So yeah, kexec + rmmod + modprobe
breaks. AFAICT.

>>> As it has been indicated previously, the flag is cleared across a low 
>>>  power transition. Therefore, the driver should set the mac address 
>>> back  into the reversed order when clearing the flag.
>>
>>
>> That's what nv_remove is supposed to do. Is there a case where nv_remove
>> is not called?
>>
>
> Sorry for the confusion. I was merely stating what needs to be done as  
> the full solution. This logic was already in place by your patch.
>
>>
>>> Also, the driver should set back the flag after a low power 
>>> transition  to protect against kexec command calling nv_probe a 
>>> second time.
>>
>>
>> Sounds like suspend stopped calling nv_remove? That would make sense
>> then. I never checked whether suspend ever actually did call nv_remove
>> (I think), but as my patch worked, it basically must have done so, at
>> least in the past, right?
>>
>
> My understanding is that nv_suspend will call nv_close and then  
> nv_resume will call nv_open. I don't think nv_probe/nv_remove is called  
> during the low power transitions.

Hm, then I fail to see why my patch had any effect. I only touched
nv_probe and nv_remove, and it solved the mac reversal on suspend
problem... *confused*

> We want to set back the flag in nv_resume in case kexec is call after  
> anytime nv_resume is called. Otherwise, nv_probe (via kexec ?) will  
> think it needs to reverse the address.

Hmhm, that also somehow conflicts with the fact that my patch did
anything... I think. 

Björn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html