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) {
[PATCH net] udp: preserve skb->dst if required for IP options processing
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")
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) {
--
2.9.4
