netdev
[Top] [All Lists]

Re: pskb change in dst->output

To: "David S. Miller" <davem@xxxxxxxxxx>
Subject: Re: pskb change in dst->output
From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Date: Thu, 8 Jul 2004 10:35:54 +1000
Cc: jmorris@xxxxxxxxxx, netdev@xxxxxxxxxxx
In-reply-to: <20040707171759.025eb13b.davem@redhat.com>
References: <20040707231221.GA30565@gondor.apana.org.au> <Xine.LNX.4.44.0407071931480.28863-100000@thoron.boston.redhat.com> <20040708000421.GA30918@gondor.apana.org.au> <20040707171759.025eb13b.davem@redhat.com>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.6+20040523i
On Wed, Jul 07, 2004 at 05:17:59PM -0700, David S. Miller wrote:
> 
> If this goes through netfilter, before James's changes, in any way
> shape or form, the checksum of the original SKB will be corrupted.
> This breaks dhcp for example, and it's really common because if you
> just build selinux even without using any rules, packets go through
> netfilter.

Thanks, I've got no problems with the netfilter part of the change
at all.

What I am trying to find out is what has this got to do with dst_output().

Let's take ah_output() as an example.  At the top of the function we've
got a conditional call to skb_checksum_help() which will unclone the
skb.  This ostensibly allows us to call ah_output() with a cloned skb.

However, if we look further down in ah_output() we will find code that
modifies the contents of skb->data without uncloning it.  So if
someone calls ah_output() with a cloned packet that does not require
skb_checksum_help() this will blow up.  The TCP case is safe as the
skb it provides is practically uncloned as long as you don't touch the
TCP payload.

This works currently as all callers to dst_output() provide it with
an skb that is practically uncloned in the sense that you can modify
all the headers at will.  But if that is the case, I don't see why
dst_output() needs to take a pskb instead of a plain skb.

Of course the current call to skb_checksum_help() will unclone
packets such as those generated by TCP, but I think that we ought
to have a version of it that doesn't unclone the packet and call
that in dst->output() functions like ah_output().

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

<Prev in Thread] Current Thread [Next in Thread>