Re: bug in ip -s xfrm state list

Re: bug in ip -s xfrm state list

[Stephen Hemminger]

On Wed, 30 May 2012 15:00:05 +0200
Jaroslav Šafka <jaroslav.safka@siemens.com> wrote:

> Hello guys,
> I found small problem in printing "seq" number.
> I think the problem is visible in diff below ;-)
> 
> Best regards
> Jarek
> 
> diff --git a/ip/ipxfrm.c b/ip/ipxfrm.c
> index c7b3420..67fe838 100644
> --- a/ip/ipxfrm.c
> +++ b/ip/ipxfrm.c
> @@ -843,7 +843,7 @@ void xfrm_state_info_print(struct xfrm_usersa_info 
> *xsinfo,
>         fputs(buf, fp);
>         fprintf(fp, "replay-window %u ", xsinfo->replay_window);
>         if (show_stats > 0)
> -               fprintf(fp, "seq 0x%08u ", xsinfo->seq);
> +               fprintf(fp, "seq %08u ", ntohl(xsinfo->seq));
>         if (show_stats > 0 || xsinfo->flags) {
>                 __u8 flags = xsinfo->flags;
>  
> 
> 

Moving to netdev list for more feedback.
Your change has two components:
  1. printing sequence number in decimal notation with 0x prefix.
  2. doing ntohl().

The first is an obvious bug in the original code. Not sure about
the second. It looks like sequence numbers generated by the kernel
come from xfrm_get_acqseq() which generates sequence numbers in host
byte order. But sequence numbers entered from user space (vi xfrm_seq_parse)
are encoded in network byte order. This looks like a flaw in the original design.

The kernel networking code tries to be careful about documenting network
verus host byte order via the typedef __be32 vs __u32.  Given that the kernel
is using __u32 for sequence number in xfrm fairly consistently; my opninon
is that iproute2 should change and go with the kernel practice and
use host byte order.