[PATCH] Change xt_TOS v1 target to zero-out semantic

[PATCH] Change xt_TOS v1 target to zero-out semantic

[Jan Engelhardt]

This patch changes the behavior of xt_TOS v1 so that the mask value
one supplies means "zero out these bits" rather than "keep these
bits". This is more easy on the user, as (I would assume) people
keep more bits than zeroing, so, an example:

	Action: Set bit 0x01.
	before: iptables -j TOS --set-tos 0x01/0xFE
	after:  iptables -j TOS --set-tos 0x01/0x01

This is not too "tragic" with xt_TOS, but where larger fields are
used (e.g. proposed xt_MARK v2), `--set-xmark 0x01/0xFFFFFFFE` vs.
`--set-xmark 0x01/0x01` is really a worthy difference.
Other modules, such as xt_TPROXY also use &~ rather than &, so
let's find a common ground.

(We can apply this patch because xt_TOS v1 is currently only in the
development tree, not mainline.)

Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>

---
 net/netfilter/xt_DSCP.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/net/netfilter/xt_DSCP.c
===================================================================
--- linux-2.6.orig/net/netfilter/xt_DSCP.c
+++ linux-2.6/net/netfilter/xt_DSCP.c
@@ -128,7 +128,7 @@ tos_tg(struct sk_buff *skb, const struct
 	u_int8_t orig, nv;
 
 	orig = ipv4_get_dsfield(iph);
-	nv   = (orig & info->tos_mask) ^ info->tos_value;
+	nv   = (orig & ~info->tos_mask) ^ info->tos_value;
 
 	if (orig != nv) {
 		if (!skb_make_writable(skb, sizeof(struct iphdr)))

Re: [PATCH] Change xt_TOS v1 target to zero-out semantic

[Patrick McHardy]

Jan Engelhardt wrote:
> This patch changes the behavior of xt_TOS v1 so that the mask value
> one supplies means "zero out these bits" rather than "keep these
> bits". This is more easy on the user, as (I would assume) people
> keep more bits than zeroing, so, an example:
> 
> 	Action: Set bit 0x01.
> 	before: iptables -j TOS --set-tos 0x01/0xFE
> 	after:  iptables -j TOS --set-tos 0x01/0x01
> 
> This is not too "tragic" with xt_TOS, but where larger fields are
> used (e.g. proposed xt_MARK v2), `--set-xmark 0x01/0xFFFFFFFE` vs.
> `--set-xmark 0x01/0x01` is really a worthy difference.
> Other modules, such as xt_TPROXY also use &~ rather than &, so
> let's find a common ground.

I'm going to apply this, but only if we're going to have
an easier to use userspace extension for this.

I'd prefer:

--set-tos: set exact value, no mask
--or-tos: set single bits
--xor-tos: flip single bits
--and-tos: mask single bits

Re: [PATCH] Change xt_TOS v1 target to zero-out semantic

[Jan Engelhardt]

On Dec 17 2007 14:04, Patrick McHardy wrote:
> Jan Engelhardt wrote:
>> This patch changes the behavior of xt_TOS v1 so that the mask value
>> one supplies means "zero out these bits" rather than "keep these
>> bits". This is more easy on the user, as (I would assume) people
>> keep more bits than zeroing, so, an example:
>> 
>>  Action: Set bit 0x01.
>>  before: iptables -j TOS --set-tos 0x01/0xFE
>>  after:  iptables -j TOS --set-tos 0x01/0x01
>> 
>> This is not too "tragic" with xt_TOS, but where larger fields are
>> used (e.g. proposed xt_MARK v2), `--set-xmark 0x01/0xFFFFFFFE` vs.
>> `--set-xmark 0x01/0x01` is really a worthy difference.
>> Other modules, such as xt_TPROXY also use &~ rather than &, so
>> let's find a common ground.
>
> I'm going to apply this, but only if we're going to have
> an easier to use userspace extension for this.
>
> I'd prefer:
>
> --set-tos: set exact value, no mask
> --or-tos: set single bits
> --xor-tos: flip single bits
> --and-tos: mask single bits
>

Ok, I'll add these. However, since they get transformed to xmark 
internally, they would normally be displayed as

	--set-tos value/mask (iptables-save, iptables -nL)
	TOS set value/mask (iptables -L)

Is that ok, or should I figure out some math to transform it back to 
--{or,xor,and}-tos for the human-readable (iptables -L) case?

Re: [PATCH] Change xt_TOS v1 target to zero-out semantic

[Patrick McHardy]

Jan Engelhardt wrote:
> On Dec 17 2007 14:04, Patrick McHardy wrote:
>
>> I'd prefer:
>>
>> --set-tos: set exact value, no mask
>> --or-tos: set single bits
>> --xor-tos: flip single bits
>> --and-tos: mask single bits
>>
> 
> Ok, I'll add these. However, since they get transformed to xmark 
> internally, they would normally be displayed as
> 
> 	--set-tos value/mask (iptables-save, iptables -nL)
> 	TOS set value/mask (iptables -L)
> 
> Is that ok, or should I figure out some math to transform it back to 
> --{or,xor,and}-tos for the human-readable (iptables -L) case?


That would be better. The mark patch I pointer you to some
time ago did this:

+print_mark(unsigned long mark, unsigned long mask, int numeric)
  {
-       printf("0x%lx ", mark);
+       if (mask == 0)
+               printf("set 0x%lx ", mark);
+       else if (mark == ~mask)
+               printf("or 0x%lx ", mark);
+       else if ((mark & mask) == 0)
+               printf("set 0x%lx/0x%lx ", mark, ~mask);
+       else {
+               if (mask != ~0UL)
+                       printf("and 0x%lx ", mask);
+               if (mark)
+                       printf("xor 0x%lx ", mark);
+       }
  }

which I guess would also work for tos.