Moving frags and SKBTX_DEV_ZEROCOPY skbs

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?

Quoted 13 lines

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?

For skb_shift(), it is currently used from tcp stack only, where this SKBTX_DEV_ZEROCOPY thing is not used, so I do not think there is a bug for the moment.

I already gave a patch for skb_try_coalesce() : For this one we do not wan skb_orphan_frags() overhead. Its simply better in this case to abort.

From: Eric Dumazet <[email protected]>
Date: Wed, 14 May 2014 07:23:52 -0700

Quoted 20 lines · Zoltan Kiss

Quoted 13 lines

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?

For skb_shift(), it is currently used from tcp stack only, where this SKBTX_DEV_ZEROCOPY thing is not used, so I do not think there is a bug for the moment.

Quoted 3 lines

I already gave a patch for skb_try_coalesce() : For this one we do not wan skb_orphan_frags() overhead. Its simply better in this case to abort.

Eric can you please submit this formally? It is second time I've seen it posted as RFC :-)

From: Eric Dumazet <[email protected]>
Date: Wed, 14 May 2014 07:23:52 -0700

Quoted 20 lines · Zoltan Kiss

Quoted 13 lines

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?

For skb_shift(), it is currently used from tcp stack only, where this SKBTX_DEV_ZEROCOPY thing is not used, so I do not think there is a bug for the moment.

Quoted 3 lines

I already gave a patch for skb_try_coalesce() : For this one we do not wan skb_orphan_frags() overhead. Its simply better in this case to abort.

Eric can you please submit this formally? It is second time I've seen it posted as RFC :-)

Quoted 13 lines

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?

For skb_shift(), it is currently used from tcp stack only, where this SKBTX_DEV_ZEROCOPY thing is not used, so I do not think there is a bug for the moment.

I already gave a patch for skb_try_coalesce() : For this one we do not wan skb_orphan_frags() overhead. Its simply better in this case to abort.

Quoted 13 lines · Zoltan Kiss

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?

For skb_shift(), it is currently used from tcp stack only, where this SKBTX_DEV_ZEROCOPY thing is not used, so I do not think there is a bug for the moment.

I already gave a patch for skb_try_coalesce() : For this one we do not wan skb_orphan_frags() overhead. Its simply better in this case to abort.

net/core/skbuff.c +4 -1

index 1b62343f5837..85995a14aafc 100644
@@ -3838,7 +3839,10 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
  		return true;
  	}
 
-	if (skb_has_frag_list(to) || skb_has_frag_list(from))
+	if (skb_has_frag_list(to) ||
+	    skb_has_frag_list(from) ||
+	    (skb_shinfo(to)->tx_flags & SKBTX_DEV_ZEROCOPY) ||
+	    (skb_shinfo(from)->tx_flags & SKBTX_DEV_ZEROCOPY))
  		return false;
 
  	if (skb_headlen(from) != 0) {

Quoted 16 lines · Eric Dumazet

Quoted 13 lines · Zoltan Kiss

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?

Quoted 3 lines

For skb_shift(), it is currently used from tcp stack only, where this SKBTX_DEV_ZEROCOPY thing is not used, so I do not think there is a bug for the moment.

It is called from tcp_input.c, which suggests it can be called on incoming TCP packets.

Quoted 16 lines · Eric Dumazet

Quoted 13 lines · Zoltan Kiss

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?

Quoted 3 lines

For skb_shift(), it is currently used from tcp stack only, where this SKBTX_DEV_ZEROCOPY thing is not used, so I do not think there is a bug for the moment.

It is called from tcp_input.c, which suggests it can be called on incoming TCP packets.

But here is the thing: deliver_skb calls orphan_frags for every packet delivered to the local stack, so we are safe IF these functions are called before the IP stack. So we are safe now, but things can go wrong, if:
- such a frag-mangling function is called before deliver_skb, now or in the future
- if someone wants to take advantage of zerocopy in the guest<->backend path

But here is the thing: deliver_skb calls orphan_frags for every packet delivered to the local stack, so we are safe IF these functions are called before the IP stack. So we are safe now, but things can go wrong, if:
- such a frag-mangling function is called before deliver_skb, now or in the future
- if someone wants to take advantage of zerocopy in the guest<->backend path

Quoted 13 lines · Zoltan Kiss

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?

For skb_shift(), it is currently used from tcp stack only, where this SKBTX_DEV_ZEROCOPY thing is not used, so I do not think there is a bug for the moment.

I already gave a patch for skb_try_coalesce() : For this one we do not wan skb_orphan_frags() overhead. Its simply better in this case to abort.

net/core/skbuff.c +4 -1

index 1b62343f5837..85995a14aafc 100644
@@ -3838,7 +3839,10 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
  		return true;
  	}
 
-	if (skb_has_frag_list(to) || skb_has_frag_list(from))
+	if (skb_has_frag_list(to) ||
+	    skb_has_frag_list(from) ||
+	    (skb_shinfo(to)->tx_flags & SKBTX_DEV_ZEROCOPY) ||
+	    (skb_shinfo(from)->tx_flags & SKBTX_DEV_ZEROCOPY))
  		return false;
 
  	if (skb_headlen(from) != 0) {

Hi,

Recently I've investigated issues around SKBTX_DEV_ZEROCOPY skbs where the frags list were modified. I came across this function skb_shift(), which moves frags between skbs. And there are a lot more of such kind, skb_split or skb_try_coalesce, for example.
It could be a dangerous thing if a frag is referenced from an skb which doesn't have the original destructor_arg, and to avoid that skb_orphan_frags should be called. Although probably these functions are not normally touched in usual usecases, I think it would be useful to review core skb functions proactively and add an skb_orphan_frags everywhere where the frags could be referenced from other places. Any opinion about this?