Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice

On Wed, 2011-05-18 at 19:36 +0300, Michael S. Tsirkin wrote:

On Wed, May 18, 2011 at 09:07:37AM -0700, Shirley Ma wrote:

quoted

On Wed, 2011-05-18 at 18:47 +0300, Michael S. Tsirkin wrote:

quoted

On Wed, May 18, 2011 at 07:38:27AM -0700, Shirley Ma wrote:

quoted

On Wed, 2011-05-18 at 13:40 +0200, Michał Mirosław wrote:

quoted

quoted

quoted

quoted

quoted

Not more other restrictions, skb clone is OK.

pskb_expand_head()

quoted

quoted

looks

quoted

quoted

quoted

quoted

OK to me from code review.

Hmm. pskb_expand_head calls skb_release_data while

keeping

quoted

quoted

quoted

quoted

quoted

quoted

quoted

references to pages. How is that ok? What do I miss?

It's making copy of the skb_shinfo earlier, so the pages

refcount

quoted

quoted

quoted

quoted

stays the same.

Exactly. But the callback is invoked so the guest thinks

it's ok

quoted

quoted

to

quoted

quoted

quoted

change this memory. If it does a corrupted packet will be

sent

quoted

quoted

out.

quoted

quoted

Hmm. I tool a quick look at skb_clone(), and it looks like

this

quoted

quoted

quoted

quoted

sequence will break this scheme:

skb2 = skb_clone(skb...);
kfree_skb(skb) or pskb_expand_head(skb);  /* callback called

*/

quoted

quoted

quoted

quoted

[use skb2, pages still referenced]
kfree_skb(skb); /* callback called again */

This sequence is common in bridge, might be in other places.

Maybe this ubuf thing should just track clones? This will make

it

quoted

quoted

work

quoted

quoted

on all devices then.

The callback was only invoked when last reference of skb was

gone.

quoted

quoted

quoted

skb_clone does increase skb refcnt. I tested tcpdump on lower

device, it

quoted

worked.

Right, it will normally work, but two issues I think you miss:
1. malicious guest can change the memory between when it is sent

out

quoted

quoted

by
   device and consumed by tcpdump, so you will see different

things

quoted

quoted

   (not sure how important this is).
2. if tcpdump stops consuming stuff from the packet socket (it's
   userspace, can't be trusted) then we won't get a callback for
   page potentially forever, guest networking will get blocked

etc.

quoted

quoted

quoted

For the sequence of:

skb_clone  -> last refcnt + 1
kfree_skb() or pskb_expand_head -> callback not called
kfree_skb() -> callback called

I will check page refcount to see whether it's balanced.

Thanks
shirley

pskb_expand_head is a problem anyway I think as it
can hang on to pages after it calls release_data.
Then guest will modify these pages and you get trash there. 

This can be avoid by allowing pskb_expand_head in fastpath only, I
think. But not sure whether tcpdump can still work with this.

Thanks
Shirley

Yes, I agree.  I think for tcpdump, we really need to copy the data
anyway, to avoid guest changing it in between.  So we do that and then
use the copy everywhere, release the old one. Hmm? 

Yes. Old one use zerocopy, new one use copy data.

Thanks
Shirley