On 6/25/19 12:59 PM, Florian Westphal wrote:
> Eric Dumazet <eric.duma...@gmail.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;
