Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

2018-06-14 Thread David Ahern
On 6/13/18 11:09 PM, Subash Abhinov Kasiviswanathan wrote:
> The output mark differs from the existing xfrm mark in two ways:
> 
> 1. The xfrm mark is used to match xfrm policies and states, while
>    the xfrm output mark is used to set the mark (and influence
>    the routing) of the packets emitted by those states.
> 2. The existing mark is constrained to be a subset of the bits of
>    the originating socket or transformed packet, but the output
>    mark is arbitrary and depends only on the state.

make sure the man page update contains the description of the 2 marks
and why they are used.

Thanks,


Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

2018-06-13 Thread Subash Abhinov Kasiviswanathan

any reason to put output-mark on its own line? Why not
mark 0x1/0x3 output-mark 0x2



Hi David

I will move it to the same line in v3.

is the documentation clear on the difference between mark and 
output-mark?


Lorenzo has described the differences in detail in the kernel commit I
had listed in the commit text of this patch. Pasting from there -

The output mark differs from the existing xfrm mark in two ways:

1. The xfrm mark is used to match xfrm policies and states, while
   the xfrm output mark is used to set the mark (and influence
   the routing) of the packets emitted by those states.
2. The existing mark is constrained to be a subset of the bits of
   the originating socket or transformed packet, but the output
   mark is arbitrary and depends only on the state.

The use of a separate mark provides additional flexibility. For
example:

- A packet subject to two transforms (e.g., transport mode inside
  tunnel mode) can have two different output marks applied to it,
  one for the transport mode SA and one for the tunnel mode SA.
- On a system where socket marks determine routing, the packets
  emitted by an IPsec tunnel can be routed based on a mark that
  is determined by the tunnel, not by the marks of the
  unencrypted packets.
- Support for setting the output marks can be introduced without
  breaking any existing setups that employ both mark-based
  routing and xfrm tunnel mode. Simply changing the code to use
  the xfrm mark for routing output packets could xfrm mark could
  change behaviour in a way that breaks these setups.






@@ -61,6 +61,7 @@ static void usage(void)
fprintf(stderr, "[ flag FLAG-LIST ] [ sel SELECTOR ] 
[ LIMIT-LIST ] [ encap ENCAP ]\n");
fprintf(stderr, "[ coa ADDR[/PLEN] ] [ ctx CTX ] [ 
extra-flag EXTRA-FLAG-LIST ]\n");

fprintf(stderr, "[ offload [dev DEV] dir DIR ]\n");
+   fprintf(stderr, "[ output-mark OUTPUT-MARK]\n");


Nit: I think you want a space between OUTPUT-MARK and ].


yes.



I will update this as well.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project


Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

2018-06-13 Thread David Ahern
On 6/12/18 9:14 PM, Lorenzo Colitti wrote:
> On Wed, Jun 13, 2018 at 3:48 AM Subash Abhinov Kasiviswanathan
>  wrote:
>>
>> src 192.168.1.1 dst 192.168.1.2
>> proto esp spi 0x4321 reqid 0 mode tunnel
>> replay-window 0 flag af-unspec
>> mark 0x1/0x3
>> output-mark 0x2
> 
> Nit: I don't know what guarantees we provide (if any) that the output
> format of "ip xfrm state" does not change except to add new lines at
> the end. Personally, I feel that an app or script that depends on
> "auth-trunc" (or anything else, really) being on the line immediately
> after "mark" is brittle and should be fixed. This is particularly true
> since in general between the mark and the encryption there might be an
> auth-trunc line, or an auth line, or neither. As such, adding this
> line here seems OK to me.

any reason to put output-mark on its own line? Why not
mark 0x1/0x3 output-mark 0x2

is the documentation clear on the difference between mark and output-mark?

> 
>> @@ -61,6 +61,7 @@ static void usage(void)
>> fprintf(stderr, "[ flag FLAG-LIST ] [ sel SELECTOR ] [ 
>> LIMIT-LIST ] [ encap ENCAP ]\n");
>> fprintf(stderr, "[ coa ADDR[/PLEN] ] [ ctx CTX ] [ 
>> extra-flag EXTRA-FLAG-LIST ]\n");
>> fprintf(stderr, "[ offload [dev DEV] dir DIR ]\n");
>> +   fprintf(stderr, "[ output-mark OUTPUT-MARK]\n");
> 
> Nit: I think you want a space between OUTPUT-MARK and ].

yes.

> 
> Other than that,
> 
> Acked-by: Lorenzo Colitti 
> 



Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

2018-06-12 Thread Stephen Hemminger
On Wed, 13 Jun 2018 12:14:53 +0900
Lorenzo Colitti  wrote:

> On Wed, Jun 13, 2018 at 3:48 AM Subash Abhinov Kasiviswanathan
>  wrote:
> >
> > src 192.168.1.1 dst 192.168.1.2
> > proto esp spi 0x4321 reqid 0 mode tunnel
> > replay-window 0 flag af-unspec
> > mark 0x1/0x3
> > output-mark 0x2  
> 
> Nit: I don't know what guarantees we provide (if any) that the output
> format of "ip xfrm state" does not change except to add new lines at
> the end. Personally, I feel that an app or script that depends on
> "auth-trunc" (or anything else, really) being on the line immediately
> after "mark" is brittle and should be fixed. This is particularly true
> since in general between the mark and the encryption there might be an
> auth-trunc line, or an auth line, or neither. As such, adding this
> line here seems OK to me.

Scripts should use json mode. If it ever gets added to xfrm output (hint).


Re: [PATCH iproute2-next v2] ip-xfrm: Add support for OUTPUT_MARK

2018-06-12 Thread Lorenzo Colitti
On Wed, Jun 13, 2018 at 3:48 AM Subash Abhinov Kasiviswanathan
 wrote:
>
> src 192.168.1.1 dst 192.168.1.2
> proto esp spi 0x4321 reqid 0 mode tunnel
> replay-window 0 flag af-unspec
> mark 0x1/0x3
> output-mark 0x2

Nit: I don't know what guarantees we provide (if any) that the output
format of "ip xfrm state" does not change except to add new lines at
the end. Personally, I feel that an app or script that depends on
"auth-trunc" (or anything else, really) being on the line immediately
after "mark" is brittle and should be fixed. This is particularly true
since in general between the mark and the encryption there might be an
auth-trunc line, or an auth line, or neither. As such, adding this
line here seems OK to me.

> @@ -61,6 +61,7 @@ static void usage(void)
> fprintf(stderr, "[ flag FLAG-LIST ] [ sel SELECTOR ] [ 
> LIMIT-LIST ] [ encap ENCAP ]\n");
> fprintf(stderr, "[ coa ADDR[/PLEN] ] [ ctx CTX ] [ extra-flag 
> EXTRA-FLAG-LIST ]\n");
> fprintf(stderr, "[ offload [dev DEV] dir DIR ]\n");
> +   fprintf(stderr, "[ output-mark OUTPUT-MARK]\n");

Nit: I think you want a space between OUTPUT-MARK and ].

Other than that,

Acked-by: Lorenzo Colitti