Re: [PATCH net] udp: preserve skb->dst if required for IP options processing
On Mon, 2017-07-17 at 14:26 -0700, Eric Dumazet wrote:
> On Mon, 2017-07-17 at 22:59 +0200, Paolo Abeni wrote:
> > Eric noticed that in udp_recvmsg() we still need to access
> > skb->dst while processing the IP options.
> > Since commit 0a463c78d25b ("udp: avoid a cache miss on dequeue")
> > skb->dst is no more available at recvmsg() time and bad things
> > will happen if we enter the relevant code path.
> >
> > This commit address the issue, avoid clearing skb->dst if
> > any IP options are present into the relevant skb.
> > Since the IP CB is contained in the first skb cacheline, we can
> > test it to decide to leverage the consume_stateless_skb()
> > optimization, without measurable additional cost in the faster
> > path.
> >
> > Fixes: b65ac44674dd ("udp: try to avoid 2 cache miss on dequeue")
>
> Strange I thought bug was coming from
> commit 0a463c78d25b ("udp: avoid a cache miss on dequeue")
You are right - and I was wrong with this tag.
>
> Please also add original reporter, from syzkaller team.
>
> Reported-by: Andrey Konovalov
I'll resubmit soon a v2 with the update tags. I'm not sure if I must or
must not retain your ack ?
BTW I think that __ip_options_echo() also needs to access skb->dev, via
fib_compute_spec_dst(), and perhaps another patch is needed. I'll look
at that today.
Thanks,
Paolo
Re: [PATCH net] udp: preserve skb->dst if required for IP options processing
On Mon, 2017-07-17 at 14:30 -0700, Eric Dumazet wrote:
> On Mon, 2017-07-17 at 22:59 +0200, Paolo Abeni wrote:
> > Eric noticed that in udp_recvmsg() we still need to access
> > skb->dst while processing the IP options.
> > Since commit 0a463c78d25b ("udp: avoid a cache miss on dequeue")
> > skb->dst is no more available at recvmsg() time and bad things
> > will happen if we enter the relevant code path.
> >
> > This commit address the issue, avoid clearing skb->dst if
> > any IP options are present into the relevant skb.
> > Since the IP CB is contained in the first skb cacheline, we can
> > test it to decide to leverage the consume_stateless_skb()
> > optimization, without measurable additional cost in the faster
> > path.
>
> Also the problem was not skb->dst at all, but fact that
> skb->head can not be freed if IP options need later to be fetched.
>
> ( __ip_options_echo() ->
>sptr = skb_network_header(skb);
>
> ... -> use after free
Oh well, I read again the whole thing, and it seems you are right.
Acked-by: Eric Dumazet
Re: [PATCH net] udp: preserve skb->dst if required for IP options processing
On Mon, 2017-07-17 at 22:59 +0200, Paolo Abeni wrote:
> Eric noticed that in udp_recvmsg() we still need to access
> skb->dst while processing the IP options.
> Since commit 0a463c78d25b ("udp: avoid a cache miss on dequeue")
> skb->dst is no more available at recvmsg() time and bad things
> will happen if we enter the relevant code path.
>
> This commit address the issue, avoid clearing skb->dst if
> any IP options are present into the relevant skb.
> Since the IP CB is contained in the first skb cacheline, we can
> test it to decide to leverage the consume_stateless_skb()
> optimization, without measurable additional cost in the faster
> path.
Also the problem was not skb->dst at all, but fact that
skb->head can not be freed if IP options need later to be fetched.
( __ip_options_echo() ->
sptr = skb_network_header(skb);
... -> use after free
Re: [PATCH net] udp: preserve skb->dst if required for IP options processing
On Mon, 2017-07-17 at 22:59 +0200, Paolo Abeni wrote:
> Eric noticed that in udp_recvmsg() we still need to access
> skb->dst while processing the IP options.
> Since commit 0a463c78d25b ("udp: avoid a cache miss on dequeue")
> skb->dst is no more available at recvmsg() time and bad things
> will happen if we enter the relevant code path.
>
> This commit address the issue, avoid clearing skb->dst if
> any IP options are present into the relevant skb.
> Since the IP CB is contained in the first skb cacheline, we can
> test it to decide to leverage the consume_stateless_skb()
> optimization, without measurable additional cost in the faster
> path.
>
> Fixes: b65ac44674dd ("udp: try to avoid 2 cache miss on dequeue")
Strange I thought bug was coming from
commit 0a463c78d25b ("udp: avoid a cache miss on dequeue")
Please also add original reporter, from syzkaller team.
Reported-by: Andrey Konovalov
> Reported-by: Eric Dumazet
> Signed-off-by: Paolo Abeni
> ---
> net/ipv4/udp.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 25294d4..b057653 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1388,6 +1388,11 @@ void skb_consume_udp(struct sock *sk, struct sk_buff
> *skb, int len)
> unlock_sock_fast(sk, slow);
> }
>
> + /* we cleared the head states previously only if the skb lacks any IP
> + * options, see __udp_queue_rcv_skb().
> + */
> + if (unlikely(IPCB(skb)->opt.optlen > 0))
> + skb_release_head_state(skb);
> consume_stateless_skb(skb);
> }
> EXPORT_SYMBOL_GPL(skb_consume_udp);
> @@ -1779,8 +1784,12 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct
> sk_buff *skb)
> sk_mark_napi_id_once(sk, skb);
> }
>
> - /* clear all pending head states while they are hot in the cache */
> - skb_release_head_state(skb);
> + /* At recvmsg() time we need skb->dst to process IP options-related
> + * cmsg, elsewhere can we clear all pending head states while they are
> + * hot in the cache
> + */
> + if (likely(IPCB(skb)->opt.optlen == 0))
> + skb_release_head_state(skb);
>
> rc = __udp_enqueue_schedule_skb(sk, skb);
> if (rc < 0) {
