[PATCH net-next 0/3] IPComp fixes

[PATCH net-next 0/3] IPComp fixes

[Fan Du]

Hi,

This patchset includes bug fix focusing on IPComp mainly, which addresses
IPComp Compression Parameter Index(CPI, 16bits of 32bits SPI value) problem.

And other issue is the compression part, undesirable packet will not be
compressed, and then send into wire, this packet will be dropped by policy
checking in the receiver side.

Fan Du (3):
  xfrm: check user specified spi for IPComp
  xfrm: clamp down spi range for IPComp when allocating spi
  xfrm: Restrict "level use" for IPComp configuration

 net/key/af_key.c      |    6 ++++++
 net/xfrm/xfrm_state.c |   13 +++++++++++++
 net/xfrm/xfrm_user.c  |    7 ++++++-
 3 files changed, 25 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[PATCH net-next 1/3] xfrm: check user specified spi for IPComp

[Fan Du]

IPComp connection between two hosts is broken if given spi bigger
than 0xffff.

OUTSPI=0x87
INSPI=0x11112

ip xfrm policy update dst 192.168.1.101 src 192.168.1.109 dir out action allow \
       tmpl dst 192.168.1.101 src 192.168.1.109 proto comp spi $OUTSPI
ip xfrm policy update src 192.168.1.101 dst 192.168.1.109 dir in action allow \
       tmpl src 192.168.1.101 dst 192.168.1.109 proto comp spi $INSPI

ip xfrm state add src 192.168.1.101 dst 192.168.1.109  proto comp spi $INSPI \
		comp deflate "0x1111"
ip xfrm state add dst 192.168.1.101 src 192.168.1.109  proto comp spi $OUTSPI \
		comp deflate "0x1111"

tcpdump can capture outbound ping packet, but inbound packet is
dropped with XfrmOutNoStates errors. It looks like spi value used
for IPComp is expected to be 16bits wide only.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/xfrm/xfrm_user.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index f964d4c..52efe71 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -209,7 +209,8 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
 		    attrs[XFRMA_ALG_AUTH]	||
 		    attrs[XFRMA_ALG_AUTH_TRUNC]	||
 		    attrs[XFRMA_ALG_CRYPT]	||
-		    attrs[XFRMA_TFCPAD])
+		    attrs[XFRMA_TFCPAD] ||
+			(ntohl(p->id.spi) >= 0x10000))
 			goto out;
 		break;
 
-- 
1.7.9.5

[PATCH net-next 2/3] xfrm: clamp down spi range for IPComp when allocating spi

[Fan Du]

otherwise xfrm state can not be found properly by peers.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/xfrm/xfrm_state.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 68c2f35..a6716d7 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1506,6 +1506,19 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
 	__be32 maxspi = htonl(high);
 	u32 mark = x->mark.v & x->mark.m;
 
+	/* Compression Parameter Index(CPI) is 16bits wide
+	 * An 32 bits spi value will hash xfrm_state into wrong hash slot.
+	 * When the upper 16bits of spi values is used as CPI for the peer
+	 * to look up xfrm state, it would generate XfrmOutNoStates error,
+	 * as apparently we are looking for the wrong hash slot.
+	 *
+	 * So clamp down the spi range into only 16bits valid wide.
+	 */
+	if (x->id.proto == IPPROTO_COMP) {
+		minspi = htonl(0xc00);
+		maxspi = htonl(0xff00);
+	}
+
 	spin_lock_bh(&x->lock);
 	if (x->km.state == XFRM_STATE_DEAD)
 		goto unlock;
-- 
1.7.9.5

[PATCH net-next 3/3] xfrm: Restrict "level use" for IPComp configuration

[Fan Du]

Quote from RFC3173:
2.2. Non-Expansion Policy

   If the total size of a compressed payload and the IPComp header, as
   defined in section 3, is not smaller than the size of the original
   payload, the IP datagram MUST be sent in the original non-compressed
   form.  To clarify: If an IP datagram is sent non-compressed, no

   IPComp header is added to the datagram.  This policy ensures saving
   the decompression processing cycles and avoiding incurring IP
   datagram fragmentation when the expanded datagram is larger than the
   MTU.

   Small IP datagrams are likely to expand as a result of compression.
   Therefore, a numeric threshold should be applied before compression,
   where IP datagrams of size smaller than the threshold are sent in the
   original form without attempting compression.  The numeric threshold
   is implementation dependent.

Current IPComp implementation is indeed by the book, whileas in practice
when sending non-compressed packet to the peer(whether or not packet len
is smaller than the threshold or the compressed len is large than orignal
packet len), the packet is dropped when checking the policy as this packet
matches the selector but not coming from any XFRM layer, i.e., with no
security path. Such naked packet will not evently make it to upper layer 4.
The result is much more wired to the user when ping peer with different
palyload lenght.

Fixing this by chanlledging IPComp level against "level use" to cure this
problem.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/key/af_key.c     |    6 ++++++
 net/xfrm/xfrm_user.c |    4 ++++
 2 files changed, 10 insertions(+)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 911ef03..d37a2c1 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1895,6 +1895,12 @@ parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)
 			return -ENOBUFS;
 	}
 
+	/* IPComp requires level use option to accomodate both compressed
+	 * and non-compressed packet when checking policy.
+	 */
+	if ((t->id.proto == IPPROTO_COMP) && (t->optional == 0))
+		return -EINVAL;
+
 	/* addresses present only in tunnel mode */
 	if (t->mode == XFRM_MODE_TUNNEL) {
 		u8 *sa = (u8 *) (rq + 1);
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 52efe71..d7216ea 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1293,6 +1293,10 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
 		default:
 			return -EINVAL;
 		}
+
+		/* Refuse any IPComp conf that missing "level use" */
+		if ((ut[i].id.proto == IPPROTO_COMP) && (ut[i].optional == 0))
+			return -EINVAL;
 	}
 
 	return 0;
-- 
1.7.9.5

Re: [PATCH net-next 0/3] IPComp fixes

[Fan Du]

Hallo, Steffen

Do I need to resend this IPcomp patch set for review?

Thanks :)

On 2013年11月28日 10:52, Fan Du wrote:
> Hi,
>
> This patchset includes bug fix focusing on IPComp mainly, which addresses
> IPComp Compression Parameter Index(CPI, 16bits of 32bits SPI value) problem.
>
> And other issue is the compression part, undesirable packet will not be
> compressed, and then send into wire, this packet will be dropped by policy
> checking in the receiver side.
>
> Fan Du (3):
>    xfrm: check user specified spi for IPComp
>    xfrm: clamp down spi range for IPComp when allocating spi
>    xfrm: Restrict "level use" for IPComp configuration
>
>   net/key/af_key.c      |    6 ++++++
>   net/xfrm/xfrm_state.c |   13 +++++++++++++
>   net/xfrm/xfrm_user.c  |    7 ++++++-
>   3 files changed, 25 insertions(+), 1 deletion(-)
>

-- 
浮沉随浪只记今朝笑

--fan fan

Re: [PATCH net-next 2/3] xfrm: clamp down spi range for IPComp when allocating spi

[Steffen Klassert]

On Thu, Nov 28, 2013 at 10:52:40AM +0800, Fan Du wrote:
> otherwise xfrm state can not be found properly by peers.
> 
> Signed-off-by: Fan Du <fan.du@windriver.com>
> ---
>  net/xfrm/xfrm_state.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 68c2f35..a6716d7 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1506,6 +1506,19 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
>  	__be32 maxspi = htonl(high);
>  	u32 mark = x->mark.v & x->mark.m;
>  
> +	/* Compression Parameter Index(CPI) is 16bits wide
> +	 * An 32 bits spi value will hash xfrm_state into wrong hash slot.
> +	 * When the upper 16bits of spi values is used as CPI for the peer
> +	 * to look up xfrm state, it would generate XfrmOutNoStates error,
> +	 * as apparently we are looking for the wrong hash slot.
> +	 *
> +	 * So clamp down the spi range into only 16bits valid wide.
> +	 */
> +	if (x->id.proto == IPPROTO_COMP) {
> +		minspi = htonl(0xc00);
> +		maxspi = htonl(0xff00);
> +	}

This does not make sense. The spi is chosen based on minspi only
if minspi is equal to maxspi. This will be never the case for
IPPROTO_COMP with your patch.

Also, the spi range is user defined, we should respect the
users configuration if the range is valid.

Please explain your choices on the minspi and maxspi value.

Re: [PATCH net-next 1/3] xfrm: check user specified spi for IPComp

[Steffen Klassert]

On Thu, Nov 28, 2013 at 10:52:39AM +0800, Fan Du wrote:
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index f964d4c..52efe71 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -209,7 +209,8 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
>  		    attrs[XFRMA_ALG_AUTH]	||
>  		    attrs[XFRMA_ALG_AUTH_TRUNC]	||
>  		    attrs[XFRMA_ALG_CRYPT]	||
> -		    attrs[XFRMA_TFCPAD])
> +		    attrs[XFRMA_TFCPAD] ||
> +			(ntohl(p->id.spi) >= 0x10000))

Please align the above line to the rest of this block.

Re: [PATCH net-next 2/3] xfrm: clamp down spi range for IPComp when allocating spi

[Fan Du]

On 2013年12月06日 19:42, Steffen Klassert wrote:
> On Thu, Nov 28, 2013 at 10:52:40AM +0800, Fan Du wrote:
>> otherwise xfrm state can not be found properly by peers.
>>
>> Signed-off-by: Fan Du<fan.du@windriver.com>
>> ---
>>   net/xfrm/xfrm_state.c |   13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
>> index 68c2f35..a6716d7 100644
>> --- a/net/xfrm/xfrm_state.c
>> +++ b/net/xfrm/xfrm_state.c
>> @@ -1506,6 +1506,19 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
>>   	__be32 maxspi = htonl(high);
>>   	u32 mark = x->mark.v&  x->mark.m;
>>
>> +	/* Compression Parameter Index(CPI) is 16bits wide
>> +	 * An 32 bits spi value will hash xfrm_state into wrong hash slot.
>> +	 * When the upper 16bits of spi values is used as CPI for the peer
>> +	 * to look up xfrm state, it would generate XfrmOutNoStates error,
>> +	 * as apparently we are looking for the wrong hash slot.
>> +	 *
>> +	 * So clamp down the spi range into only 16bits valid wide.
>> +	 */
>> +	if (x->id.proto == IPPROTO_COMP) {
>> +		minspi = htonl(0xc00);
>> +		maxspi = htonl(0xff00);
>> +	}
>
> This does not make sense. The spi is chosen based on minspi only
> if minspi is equal to maxspi. This will be never the case for
> IPPROTO_COMP with your patch.
>
> Also, the spi range is user defined, we should respect the
> users configuration if the range is valid.

Ok, then, speaking of respect user defined range, how about below informal
patch which only check the validity of the range? My original thoughts is CPI
is only 16bits wide, kernel itself can keep the CPI's validity. btw, v2 will
also fix patch1/3 align issue.

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 6a9c402..2c6fb99 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1507,6 +1507,9 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)

         err = -ENOENT;

+       if ((x->id.proto == IPPROTO_COMP) && (high > 0xFFFF))
+               goto unlock;
+
         if (minspi == maxspi) {
                 x0 = xfrm_state_lookup(net, mark, &x->id.daddr, minspi, x->id.proto, x->props.family);
                 if (x0) {

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCH net-next 2/3] xfrm: clamp down spi range for IPComp when allocating spi

[Steffen Klassert]

On Mon, Dec 09, 2013 at 02:27:43PM +0800, Fan Du wrote:
> On 2013年12月06日 19:42, Steffen Klassert wrote:
> >
> >Also, the spi range is user defined, we should respect the
> >users configuration if the range is valid.
> 
> Ok, then, speaking of respect user defined range, how about below informal
> patch which only check the validity of the range? My original thoughts is CPI
> is only 16bits wide, kernel itself can keep the CPI's validity. btw, v2 will
> also fix patch1/3 align issue.
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 6a9c402..2c6fb99 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1507,6 +1507,9 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
> 
>         err = -ENOENT;
> 
> +       if ((x->id.proto == IPPROTO_COMP) && (high > 0xFFFF))
> +               goto unlock;
> +

This check is already done in verify_userspi_info() if xfrm_alloc_spi()
is called from xfrm_alloc_userspi().

Instead of doing this check here again, we should implement an equivalent
to verify_userspi_info() for pfkey. Then we are sure to have a valid range
in any case.

Re: [PATCH net-next 2/3] xfrm: clamp down spi range for IPComp when allocating spi

[Fan Du]

On 2013年12月09日 16:57, Steffen Klassert wrote:
> On Mon, Dec 09, 2013 at 02:27:43PM +0800, Fan Du wrote:
>> On 2013年12月06日 19:42, Steffen Klassert wrote:
>>>
>>> Also, the spi range is user defined, we should respect the
>>> users configuration if the range is valid.
>>
>> Ok, then, speaking of respect user defined range, how about below informal
>> patch which only check the validity of the range? My original thoughts is CPI
>> is only 16bits wide, kernel itself can keep the CPI's validity. btw, v2 will
>> also fix patch1/3 align issue.
>>
>> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
>> index 6a9c402..2c6fb99 100644
>> --- a/net/xfrm/xfrm_state.c
>> +++ b/net/xfrm/xfrm_state.c
>> @@ -1507,6 +1507,9 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
>>
>>          err = -ENOENT;
>>
>> +       if ((x->id.proto == IPPROTO_COMP)&&  (high>  0xFFFF))
>> +               goto unlock;
>> +
>
> This check is already done in verify_userspi_info() if xfrm_alloc_spi()
> is called from xfrm_alloc_userspi().
>
> Instead of doing this check here again, we should implement an equivalent
> to verify_userspi_info() for pfkey. Then we are sure to have a valid range
> in any case.
>

How about export an common function in xfrm_state.c to check this corner case?
This could be shared by both netlink and pfkey interface, and verify_userspi_info
simplified also?

int check_ipcomp_spirange(u8 proto, u32 high)
{
	if ((proto == IPPROTO_COMP) && (high > 0xFFFF))
		return -EINVAL;
	else return 0;
}
EXPORT_SYMBOL(check_ipcomp_spirange);



-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCH net-next 2/3] xfrm: clamp down spi range for IPComp when allocating spi

[Steffen Klassert]

On Mon, Dec 09, 2013 at 05:13:52PM +0800, Fan Du wrote:
> 
> 
> On 2013年12月09日 16:57, Steffen Klassert wrote:
> >
> >Instead of doing this check here again, we should implement an equivalent
> >to verify_userspi_info() for pfkey. Then we are sure to have a valid range
> >in any case.
> >
> 
> How about export an common function in xfrm_state.c to check this corner case?
> This could be shared by both netlink and pfkey interface, and verify_userspi_info
> simplified also?
> 
> int check_ipcomp_spirange(u8 proto, u32 high)
> {
> 	if ((proto == IPPROTO_COMP) && (high > 0xFFFF))
> 		return -EINVAL;
> 	else return 0;
> }
> EXPORT_SYMBOL(check_ipcomp_spirange);

I don't think that we should export such a function,
it is not sufficient.

The netlink interface is ok, it does verify_userspi_info(),
and the pfkey interface need all the checks done in
verify_userspi_info() too. In particular the check if
the minimum spi value is not bigger than the maximum.

So we could either make verify_userspi_info() shared,
or implement a own function for pfkey.

Re: [PATCH net-next 2/3] xfrm: clamp down spi range for IPComp when allocating spi

[Fan Du]

On 2013年12月09日 17:51, Steffen Klassert wrote:
> On Mon, Dec 09, 2013 at 05:13:52PM +0800, Fan Du wrote:
>>
>>
>> On 2013年12月09日 16:57, Steffen Klassert wrote:
>>>
>>> Instead of doing this check here again, we should implement an equivalent
>>> to verify_userspi_info() for pfkey. Then we are sure to have a valid range
>>> in any case.
>>>
>>
>> How about export an common function in xfrm_state.c to check this corner case?
>> This could be shared by both netlink and pfkey interface, and verify_userspi_info
>> simplified also?
>>
>> int check_ipcomp_spirange(u8 proto, u32 high)
>> {
>> 	if ((proto == IPPROTO_COMP)&&  (high>  0xFFFF))
>> 		return -EINVAL;
>> 	else return 0;
>> }
>> EXPORT_SYMBOL(check_ipcomp_spirange);
>
> I don't think that we should export such a function,
> it is not sufficient.
>
> The netlink interface is ok, it does verify_userspi_info(),
> and the pfkey interface need all the checks done in
> verify_userspi_info() too. In particular the check if
> the minimum spi value is not bigger than the maximum.
>
> So we could either make verify_userspi_info() shared,

Ok, I will try to export verify_userspi_info then.
Is there any comments on patch3/3 before I make v2?

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCH net-next 3/3] xfrm: Restrict "level use" for IPComp configuration

[Steffen Klassert]

On Thu, Nov 28, 2013 at 10:52:41AM +0800, Fan Du wrote:
> 
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index 911ef03..d37a2c1 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -1895,6 +1895,12 @@ parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)
>  			return -ENOBUFS;
>  	}
>  
> +	/* IPComp requires level use option to accomodate both compressed
> +	 * and non-compressed packet when checking policy.
> +	 */
> +	if ((t->id.proto == IPPROTO_COMP) && (t->optional == 0))
> +		return -EINVAL;
> +
>  	/* addresses present only in tunnel mode */
>  	if (t->mode == XFRM_MODE_TUNNEL) {
>  		u8 *sa = (u8 *) (rq + 1);
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 52efe71..d7216ea 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1293,6 +1293,10 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
>  		default:
>  			return -EINVAL;
>  		}
> +
> +		/* Refuse any IPComp conf that missing "level use" */
> +		if ((ut[i].id.proto == IPPROTO_COMP) && (ut[i].optional == 0))
> +			return -EINVAL;
>  	}

I think this will make a lot of people unhappy. It was never required
to set 'optional' for ipcomp, and I'd bet that most users don't set
it for ipcomp. I understand the problem, but we can't fix it like that.

Re: [PATCH net-next 3/3] xfrm: Restrict "level use" for IPComp configuration

[Fan Du]

On 2013年12月09日 18:38, Steffen Klassert wrote:
> On Thu, Nov 28, 2013 at 10:52:41AM +0800, Fan Du wrote:
>>
>> diff --git a/net/key/af_key.c b/net/key/af_key.c
>> index 911ef03..d37a2c1 100644
>> --- a/net/key/af_key.c
>> +++ b/net/key/af_key.c
>> @@ -1895,6 +1895,12 @@ parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)
>>   			return -ENOBUFS;
>>   	}
>>
>> +	/* IPComp requires level use option to accomodate both compressed
>> +	 * and non-compressed packet when checking policy.
>> +	 */
>> +	if ((t->id.proto == IPPROTO_COMP)&&  (t->optional == 0))
>> +		return -EINVAL;
>> +
>>   	/* addresses present only in tunnel mode */
>>   	if (t->mode == XFRM_MODE_TUNNEL) {
>>   		u8 *sa = (u8 *) (rq + 1);
>> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
>> index 52efe71..d7216ea 100644
>> --- a/net/xfrm/xfrm_user.c
>> +++ b/net/xfrm/xfrm_user.c
>> @@ -1293,6 +1293,10 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
>>   		default:
>>   			return -EINVAL;
>>   		}
>> +
>> +		/* Refuse any IPComp conf that missing "level use" */
>> +		if ((ut[i].id.proto == IPPROTO_COMP)&&  (ut[i].optional == 0))
>> +			return -EINVAL;
>>   	}
>
> I think this will make a lot of people unhappy. It was never required
> to set 'optional' for ipcomp, and I'd bet that most users don't set
> it for ipcomp. I understand the problem, but we can't fix it like that.

Instead of making this check, what about wire 'optional' to 1? it doesn't
breaking existing script.

Do you have any other way to cure this problem other than 'optional'.

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCH net-next 3/3] xfrm: Restrict "level use" for IPComp configuration

[Steffen Klassert]

On Tue, Dec 10, 2013 at 10:39:51AM +0800, Fan Du wrote:
> 
> 
> On 2013年12月09日 18:38, Steffen Klassert wrote:
> >
> >I think this will make a lot of people unhappy. It was never required
> >to set 'optional' for ipcomp, and I'd bet that most users don't set
> >it for ipcomp. I understand the problem, but we can't fix it like that.
> 
> Instead of making this check, what about wire 'optional' to 1? it doesn't
> breaking existing script.

But it might change what a user expects to happen.

> 
> Do you have any other way to cure this problem other than 'optional'.
> 

I think the user can 'fix' the problem himself by setting 'optional'.
This has also the advantage that he is aware about the change. Maybe
this should be documented somewhere.

Re: [PATCH net-next 3/3] xfrm: Restrict "level use" for IPComp configuration

[Fan Du]

On 2013年12月10日 21:11, Steffen Klassert wrote:
> On Tue, Dec 10, 2013 at 10:39:51AM +0800, Fan Du wrote:
>>
>>
>> On 2013年12月09日 18:38, Steffen Klassert wrote:
>>>
>>> I think this will make a lot of people unhappy. It was never required
>>> to set 'optional' for ipcomp, and I'd bet that most users don't set
>>> it for ipcomp. I understand the problem, but we can't fix it like that.
>>
>> Instead of making this check, what about wire 'optional' to 1? it doesn't
>> breaking existing script.
>
> But it might change what a user expects to happen.
>
>>
>> Do you have any other way to cure this problem other than 'optional'.
>>
>
> I think the user can 'fix' the problem himself by setting 'optional'.
> This has also the advantage that he is aware about the change. Maybe
> this should be documented somewhere.
>

I suspect adding a WARN in here is not good, so how about below doc looks
like?


Documentation/networking/ipsec.txt

Here documents known IPsec corner cases which need to be keep in mind when
deploy various IPsec configuration in real world production environment.

1. IPcomp: Small IP packet won't get compressed at sender, and failed on
	   policy check on receiver.

Quote from RFC3173:
2.2. Non-Expansion Policy

    If the total size of a compressed payload and the IPComp header, as
    defined in section 3, is not smaller than the size of the original
    payload, the IP datagram MUST be sent in the original non-compressed
    form.  To clarify: If an IP datagram is sent non-compressed, no

    IPComp header is added to the datagram.  This policy ensures saving
    the decompression processing cycles and avoiding incurring IP
    datagram fragmentation when the expanded datagram is larger than the
    MTU.

    Small IP datagrams are likely to expand as a result of compression.
    Therefore, a numeric threshold should be applied before compression,
    where IP datagrams of size smaller than the threshold are sent in the
    original form without attempting compression.  The numeric threshold
    is implementation dependent.

Current IPComp implementation is indeed by the book, while as in practice
when sending non-compressed packet to the peer(whether or not packet len
is smaller than the threshold or the compressed len is large than original
packet len), the packet is dropped when checking the policy as this packet
matches the selector but not coming from any XFRM layer, i.e., with no
security path. Such naked packet will not eventually make it to upper layer.
The result is much more wired to the user when ping peer with different
payload length.

One workaround is try to set "level use" for each policy if user observed
above scenario. The consequence of doing so is small packet(uncompressed)
will skip policy checking on receiver.



-- 
浮沉随浪只记今朝笑

--fan