On 6/25/19 12:22 PM, Florian Westphal wrote:
> XFRM and netfilter don't expect that skb_dst_force() can cause skb to lose
> its dst entry.
>
> I got a bug report with a skb->dst NULL dereference in netfilter
> output path. The backtrace contains nf_reinject(), so the dst
> might have been cleared when skb got queued to userspace.
>
> The xfrm part of this change was done after code inspection,
> it looks like similar crash could happen here too.
>
> One way to fix this is to add a skb_dst() check right after
> skb_dst_force() call, but I think its preferable to make the
> 'dst might get cleared' part of the function explicit.
>
> Signed-off-by: Florian Westphal <f...@strlen.de>
> ---
> include/net/dst.h | 6 +++++-
> net/netfilter/nf_queue.c | 6 +++++-
> net/xfrm/xfrm_policy.c | 5 ++++-
> 3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 12b31c602cb0..42cd53d51364 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -302,8 +302,9 @@ static inline bool dst_hold_safe(struct dst_entry *dst)
> * @skb: buffer
> *
> * If dst is not yet refcounted and not destroyed, grab a ref on it.
> + * Returns false if skb had a destroyed dst.
> */
> -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.
Say if we have two skb_dst_force() calls for some reason
on the same skb, only the first one will return false.
