Reg: Replacing sk_buff in a NetFilter hook

Reg: Replacing sk_buff in a NetFilter hook

[Subhadeep Ghosh]

Hello People,

I have written a hook for the NetFilter bridge and it works absolutely
fine. However, now some of the requirements for the hook have changed
and now I wish to perform the following operations,

1. Create a new sk_buff.
2. Copy the contents of the sk_buff, which NetFilter had passed, to
the newly created sk_buff.
3. Either mark the original sk_buff as NF_STOLEN or NF_DROP.
4. Replace the newly created sk_buff with the original sk_buff in the
NetFilter queue.

The first and the second steps are no-brainers. However, I don't know
if I need to drop the original packet or mark it as stolen. And I
definitely don't know how to resolve the fourth point. It would be
great if anyone could point me in the right direction.

Thank you and regards,
Subhadeep Ghosh.

Re: Reg: Replacing sk_buff in a NetFilter hook

[Jan Engelhardt]

On Friday 2010-04-16 12:37, Subhadeep Ghosh wrote:
>
>The first and the second steps are no-brainers. However, I don't know
>if I need to drop the original packet or mark it as stolen. And I
>definitely don't know how to resolve the fourth point. It would be
>great if anyone could point me in the right direction.

NF_DROP will cause netfilter to free it when the hooks are
done; with NF_STOLEN, you have ownership of the skb and
need to free it yourself.

Re: Reg: Replacing sk_buff in a NetFilter hook

[Subhadeep Ghosh]

Hello Jan,

Thank you for your reply. Yes, I am aware of what NF_DROP and
NF_STOLEN mean. My question was, is it possible for me to replace the
original sk_buff with the new sk_buff which I created, in the sk_buff
processing queue?

For example in the earlier NetFilter hook implementations, the
signature of a hook function was,

unsigned int nf_hookfn(unsigned int hooknum,
                                  struct sk_buff** skb,
                                  const struct net_device* in,
                                  const struct net_device* out,
                                  int (*okfn)(struct sk_buff*));

While in the current implementations, it has been changed to,

unsigned int nf_hookfn(unsigned int hooknum,
                                  struct sk_buff* skb,
                                  const struct net_device* in,
                                  const struct net_device* out,
                                  int (*okfn)(struct sk_buff*));

The only difference between the two implementations is sk_buff** has
been changed to sk_buff*. So, in the earlier versions if *skb was
assigned with the address of the newly created sk_buff and marked the
original sk_buff as NF_STOLEN and then did a kfree_skb on it, then it
would work. However, in the current implementations a hook developer
is not allowed to change the sk_buff pointer which NetFilter sends to
the hook functions. And that is what I wanted to do.

Thank you and regards,
Subhadeep Ghosh.

On Fri, Apr 16, 2010 at 4:18 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
> On Friday 2010-04-16 12:37, Subhadeep Ghosh wrote:
>>
>>The first and the second steps are no-brainers. However, I don't know
>>if I need to drop the original packet or mark it as stolen. And I
>>definitely don't know how to resolve the fourth point. It would be
>>great if anyone could point me in the right direction.
>
> NF_DROP will cause netfilter to free it when the hooks are
> done; with NF_STOLEN, you have ownership of the skb and
> need to free it yourself.
>

Re: Reg: Replacing sk_buff in a NetFilter hook

[Jan Engelhardt]

On Friday 2010-04-16 13:01, Subhadeep Ghosh wrote:

>Hello Jan,
>
>Thank you for your reply. Yes, I am aware of what NF_DROP and
>NF_STOLEN mean. My question was, is it possible for me to replace the
>original sk_buff with the new sk_buff which I created, in the sk_buff
>processing queue?
>
>For example in the earlier NetFilter hook implementations, the
>signature of a hook function was,
>
>unsigned int nf_hookfn(unsigned int hooknum,
>                                  struct sk_buff** skb,
>                                  const struct net_device* in,
>                                  const struct net_device* out,
>                                  int (*okfn)(struct sk_buff*));

This was once used because an skb could be shared, and making changes
to it in COW fashion requires the skb to be copied - something like
that, it's in the git logs (v2.6.23-3980-g3db05fe and its immediate
parent commits).
The double indirection was, I think, not originally meant
for replacing packets.

>While in the current implementations, it has been changed to,
>
>unsigned int nf_hookfn(unsigned int hooknum,
>                                  struct sk_buff* skb,
>                                  const struct net_device* in,
>                                  const struct net_device* out,
>                                  int (*okfn)(struct sk_buff*));
>
>The only difference between the two implementations is sk_buff** has
>been changed to sk_buff*. So, in the earlier versions if *skb was
>assigned with the address of the newly created sk_buff and marked the
>original sk_buff as NF_STOLEN

NF_xxx is not a property of a skb, though. The implicit stealing
of the oldskb was just a side-effect.

>and then did a kfree_skb on it, then it
>would work. However, in the current implementations a hook developer
>is not allowed to change the sk_buff pointer which NetFilter sends to
>the hook functions. And that is what I wanted to do.

If you can replace a skb, you can just as well change the existing
one.

Re: Reg: Replacing sk_buff in a NetFilter hook

[Subhadeep Ghosh]

Hello Jan,

Thank you once again for the reply. I asked the question because I was
of the notion that it was not a very good play around with the memory
which was allocated by some other NetFilter module. I had this doubt
because I got confused between NDIS network drivers, where modifying
the pointers in a packet which was allocated by some other module
resulted in a BSOD, and NetFilter modules.

Thank you once again for clarifying my doubts.

Thank you and regards,
Subhadeep Ghosh.

On Fri, Apr 16, 2010 at 5:42 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
> On Friday 2010-04-16 13:01, Subhadeep Ghosh wrote:
>
>>Hello Jan,
>>
>>Thank you for your reply. Yes, I am aware of what NF_DROP and
>>NF_STOLEN mean. My question was, is it possible for me to replace the
>>original sk_buff with the new sk_buff which I created, in the sk_buff
>>processing queue?
>>
>>For example in the earlier NetFilter hook implementations, the
>>signature of a hook function was,
>>
>>unsigned int nf_hookfn(unsigned int hooknum,
>>                                  struct sk_buff** skb,
>>                                  const struct net_device* in,
>>                                  const struct net_device* out,
>>                                  int (*okfn)(struct sk_buff*));
>
> This was once used because an skb could be shared, and making changes
> to it in COW fashion requires the skb to be copied - something like
> that, it's in the git logs (v2.6.23-3980-g3db05fe and its immediate
> parent commits).
> The double indirection was, I think, not originally meant
> for replacing packets.
>
>>While in the current implementations, it has been changed to,
>>
>>unsigned int nf_hookfn(unsigned int hooknum,
>>                                  struct sk_buff* skb,
>>                                  const struct net_device* in,
>>                                  const struct net_device* out,
>>                                  int (*okfn)(struct sk_buff*));
>>
>>The only difference between the two implementations is sk_buff** has
>>been changed to sk_buff*. So, in the earlier versions if *skb was
>>assigned with the address of the newly created sk_buff and marked the
>>original sk_buff as NF_STOLEN
>
> NF_xxx is not a property of a skb, though. The implicit stealing
> of the oldskb was just a side-effect.
>
>>and then did a kfree_skb on it, then it
>>would work. However, in the current implementations a hook developer
>>is not allowed to change the sk_buff pointer which NetFilter sends to
>>the hook functions. And that is what I wanted to do.
>
> If you can replace a skb, you can just as well change the existing
> one.
>