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  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:	Thu, 3 May 2007 18:38:34 +0200
From:	Segher Boessenkool <segher@...nel.crashing.org>
To:	Scott Wood <scottwood@...escale.com>
Cc:	jgarzik@...ox.com, linuxppc-dev@...abs.org,
	Kumar Gala <galak@...nel.crashing.org>, netdev@...r.kernel.org
Subject: Re: [PATCH v2] gianfar: Add I/O barriers when touching buffer descriptor ownership.

>> So what about some thing like this where we do the read only once?
>>
>> - k
>>
>> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
>> index a06d8d1..9cd7d1e 100644
>> --- a/drivers/net/gianfar.c
>> +++ b/drivers/net/gianfar.c
>> @@ -1438,31 +1438,35 @@ int gfar_clean_rx_ring(struct net_device 
>> *dev, int rx_work_limit)
>>  {
>>  	struct rxbd8 *bdp;
>>  	struct sk_buff *skb;
>> -	u16 pkt_len;
>> +	u16 pkt_len, status;
>> +	u32 bd_info;
>
>
> I suggested that on IRC yesterday, and Segher was concerned that the
> compiler might, in theory, "optimize" it into to two lhz instructions.

Yes.  The same is true of the original code btw, but since
you test only one bit there, all is fine.

> I'm rather skeptical that it would actually do so (even if it needs to
> load twice due to register pressure, why not just use lwz both times?),

Sure.  That doesn't make this code correct though.

> and there's probably many other places that would break if it did,

Most other network drivers read from an MMIO reg to see
which RX ring entries are kernel owned AFAICS.

> but I wasn't up for digging around GCC to prove otherwise.

It doesn't matter what current GCC does -- simply look
at what it is *allowed* to do instead.

If you want a 32-bit read to be atomic, you should
do the read via a (volatile u32 *).  Doing this with
a cast in the places where you need the atomic access
makes sure you don't get unnecessary rereads.

> Plus, that wouldn't synchronize the bd_info read with the buffer data 
> reads.

Yes, you still need the rmb().


Segher

-
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

Powered by blists - more mailing lists