netdev - Re: [PATCH net] net: make skb_dst_force return false when dst was cleared

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:   Wed, 26 Jun 2019 06:50:30 +0200
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Florian Westphal <fw@...len.de>,
        Eric Dumazet <eric.dumazet@...il.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] net: make skb_dst_force return false when dst was
 cleared



On 6/25/19 12:59 PM, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@...il.com> wrote:
>>> -static inline void skb_dst_force(struct sk_buff *skb)
>>> +static inline bool skb_dst_force(struct sk_buff *skb)
>>>  {
>>>  	if (skb_dst_is_noref(skb)) {
>>>  		struct dst_entry *dst = skb_dst(skb);
>>> @@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
>>>  			dst = NULL;
>>>  
>>>  		skb->_skb_refdst = (unsigned long)dst;
>>> +		return dst != NULL;
>>>  	}
>>> +
>>> +	return true;
>>
>> This will return true, even if skb has a NULL dst.
> 
> Yes, that was intentional -- it should return false to
> let caller know that no reference could be obtained and
> that the dst was invalidated as a result.

Problem is that some callers ignore skb_dst_force() return value.

> 
>> Say if we have two skb_dst_force() calls for some reason
>> on the same skb, only the first one will return false.
> 
> What would you suggest instead?
> 
> Alternative is something like
> 
> if (skb_dst(skb)) {
> 	skb_dst_force(skb);
> 	if (!skb_dst(skb)) {
> 		kfree_skb(skb);
> 		goto err;
> 	}
> }


Simply change 

return true;

by

return skb->_skb_refdst != 0UL;