netdev - Re: [PATCH] net: use skb_get

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:	Tue, 24 Aug 2010 21:53:49 +0800
From:	Changli Gao <xiaosuo@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: use skb_get_rxhash()

On Tue, Aug 24, 2010 at 6:21 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> Le mardi 24 août 2010 à 17:52 +0800, Changli Gao a écrit :
>> Although it is rare the line contains skb->rxhash is executed:
>>
>>  * The forwarded skbs should have the the rx queue recoreded.
>>  * The local generated skbs should have non zero sk_hash in most of cases.
>>
>> when it is executed, a non zero rxhash will be better.
>>
>> Signed-off-by: Changli Gao <xiaosuo@...il.com>
>> ---
>>  net/core/dev.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 859e30f..ea041fd 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2034,7 +2034,7 @@ u16 skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb)
>>       if (skb->sk && skb->sk->sk_hash)
>>               hash = skb->sk->sk_hash;
>>       else
>> -             hash = (__force u16) skb->protocol ^ skb->rxhash;
>> +             hash = (__force u16) skb->protocol ^ skb_get_rxhash(skb);
>>       hash = jhash_1word(hash, hashrnd);
>>
>>       return (u16) (((u64) hash * dev->real_num_tx_queues) >> 32);
>> --
>
> Instead of adding hacks and growing kernel code (considering your
> previous patch), could we sit down and fix existing code, if really
> needed ?
>
> Spirit of existing code is that tcp/udp sockets have a sk_hash field,
> and this feeds skb->rxhash in transmit path.
>
> (check commit 87fd308c for an example of a fix)
>

Thanks. Got it. skb->rxhash should be only used when skb->sk is NULL,
and skb->rxhash should save the value copied from skb->sk->sk_hash at
that time.

       if (skb->sk && skb->sk->sk_hash)
               hash = skb->sk->sk_hash;

However, do we really need to check if skb->sk->sk_hash is zero? It is
a rare case, and this check may mislead someone to think a non zero
sk_hash means that sk_hash is set.

-- 
Regards,
Changli Gao(xiaosuo@...il.com)
--
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