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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190625195943.44jvck5syvnzxb55@breakpoint.cc>
Date:   Tue, 25 Jun 2019 21:59:43 +0200
From:   Florian Westphal <fw@...len.de>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: make skb_dst_force return false when dst was
 cleared

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.

> 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;
	}
}

... i find this a bit ugly.