[PATCH net] sctp: fix SSN comparision

[PATCH net] sctp: fix SSN comparision

[Marcelo Ricardo Leitner]

This function actually operates on u32 yet its paramteres were declared
as u16, causing integer truncation upon calling.

Note in patch context that ADDIP_SERIAL_SIGN_BIT is already 32 bits.

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---

This issue exists since before git import, so I can't put a Fixes tag.
Also, that said, probably not worth queueing it to stable.
Thanks

 include/net/sctp/sm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index efc01743b9d641bf6b16a37780ee0df34b4ec698..bafe2a0ab9085f24e17038516c55c00cfddd02f4 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -382,7 +382,7 @@ enum {
 	ADDIP_SERIAL_SIGN_BIT = (1<<31)
 };
 
-static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
+static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
 {
 	return ((s) == (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
 }
-- 
2.7.4

[PATCH net] sctp: fix SSN comparision

[Marcelo Ricardo Leitner]

This function actually operates on u32 yet its paramteres were declared
as u16, causing integer truncation upon calling.

Note in patch context that ADDIP_SERIAL_SIGN_BIT is already 32 bits.

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---

This issue exists since before git import, so I can't put a Fixes tag.
Also, that said, probably not worth queueing it to stable.
Thanks

 include/net/sctp/sm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index efc01743b9d641bf6b16a37780ee0df34b4ec698..bafe2a0ab9085f24e17038516c55c00cfddd02f4 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -382,7 +382,7 @@ enum {
 	ADDIP_SERIAL_SIGN_BIT = (1<<31)
 };
 
-static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
+static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
 {
 	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
 }
-- 
2.7.4

Re: [PATCH net] sctp: fix SSN comparision

[Neil Horman]

On Thu, Sep 15, 2016 at 03:02:38PM -0300, Marcelo Ricardo Leitner wrote:
> This function actually operates on u32 yet its paramteres were declared
> as u16, causing integer truncation upon calling.
> 
> Note in patch context that ADDIP_SERIAL_SIGN_BIT is already 32 bits.
> 
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> 
> This issue exists since before git import, so I can't put a Fixes tag.
> Also, that said, probably not worth queueing it to stable.
> Thanks
> 
>  include/net/sctp/sm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index efc01743b9d641bf6b16a37780ee0df34b4ec698..bafe2a0ab9085f24e17038516c55c00cfddd02f4 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -382,7 +382,7 @@ enum {
>  	ADDIP_SERIAL_SIGN_BIT = (1<<31)
>  };
>  
> -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
>  {
>  	return ((s) == (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
>  }
> -- 
> 2.7.4
> 

Acked-by: Neil Horman <nhorman@tuxdriver.com>

> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net] sctp: fix SSN comparision

[Neil Horman]

On Thu, Sep 15, 2016 at 03:02:38PM -0300, Marcelo Ricardo Leitner wrote:
> This function actually operates on u32 yet its paramteres were declared
> as u16, causing integer truncation upon calling.
> 
> Note in patch context that ADDIP_SERIAL_SIGN_BIT is already 32 bits.
> 
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> 
> This issue exists since before git import, so I can't put a Fixes tag.
> Also, that said, probably not worth queueing it to stable.
> Thanks
> 
>  include/net/sctp/sm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index efc01743b9d641bf6b16a37780ee0df34b4ec698..bafe2a0ab9085f24e17038516c55c00cfddd02f4 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -382,7 +382,7 @@ enum {
>  	ADDIP_SERIAL_SIGN_BIT = (1<<31)
>  };
>  
> -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
>  {
>  	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
>  }
> -- 
> 2.7.4
> 

Acked-by: Neil Horman <nhorman@tuxdriver.com>

> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH net] sctp: fix SSN comparision

[David Laight]

> > -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> > +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
> >  {
> >  	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
> >  }

Does gcc manage to compile that to (s32)((t) - (s)) <= 0 ?

	David

Re: [PATCH net] sctp: fix SSN comparision

[Marcelo Ricardo Leitner]

On Fri, Sep 16, 2016 at 01:33:47PM +0000, David Laight wrote:
> > > -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> > > +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
> > >  {
> > >  	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
> > >  }
> 
> Does gcc manage to compile that to (s32)((t) - (s)) <= 0 ?

I have no idea but I have a patch in the works for updating it to this
form, to be more like time_after(). Will probably post it by next week.

  Marcelo

Re: [PATCH net] sctp: fix SSN comparision

[Marcelo Ricardo Leitner]

On Fri, Sep 16, 2016 at 10:40:47AM -0300, Marcelo Ricardo Leitner wrote:
> On Fri, Sep 16, 2016 at 01:33:47PM +0000, David Laight wrote:
> > > > -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> > > > +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
> > > >  {
> > > >  	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
> > > >  }
> > 
> > Does gcc manage to compile that to (s32)((t) - (s)) <= 0 ?
> 
> I have no idea but I have a patch in the works for updating it to this
> form, to be more like time_after(). Will probably post it by next week.

Just for reference, this should be it. Not realy tested yet.

commit b52575533dfefa1908c7bbe35bde0fa8359dd1a3
Author: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date:   Tue Aug 9 14:45:35 2016 -0300

    sctp: improve how SSN, TSN and ASCONF serial are compared
    
    Make it similar to time_before() macros instead.
    
    This patch also removes SSN_lte as it is not used.

diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index bafe2a0ab908..d30cfb6d0480 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -335,19 +335,15 @@ static inline __u16 sctp_data_size(struct sctp_chunk *chunk)
  * Number Arithmetic as defined in [RFC1982] where SERIAL_BITS = 32.
  */
 
-enum {
-	TSN_SIGN_BIT = (1<<31)
-};
+#define TSN_lt(a,b)	\
+	(typecheck(__u32, a) && \
+	 typecheck(__u32, b) && \
+	 ((__s32)((a) - (b)) < 0))
 
-static inline int TSN_lt(__u32 s, __u32 t)
-{
-	return ((s) - (t)) & TSN_SIGN_BIT;
-}
-
-static inline int TSN_lte(__u32 s, __u32 t)
-{
-	return ((s) = (t)) || (((s) - (t)) & TSN_SIGN_BIT);
-}
+#define TSN_lte(a,b)	\
+	(typecheck(__u32, a) && \
+	 typecheck(__u32, b) && \
+	 ((__s32)((a) - (b)) <= 0))
 
 /* Compare two SSNs */
 
@@ -356,36 +352,22 @@ static inline int TSN_lte(__u32 s, __u32 t)
  *  1.6 Serial Number Arithmetic
  *
  * Comparisons and arithmetic on Stream Sequence Numbers in this document
- * SHOULD use Serial Number Arithmetic as defined in [RFC1982] where
- * SERIAL_BITS = 16.
+ * SHOULD use Serial Number Arithmetic as defined in [RFC1982]
  */
-enum {
-	SSN_SIGN_BIT = (1<<15)
-};
-
-static inline int SSN_lt(__u16 s, __u16 t)
-{
-	return ((s) - (t)) & SSN_SIGN_BIT;
-}
-
-static inline int SSN_lte(__u16 s, __u16 t)
-{
-	return ((s) = (t)) || (((s) - (t)) & SSN_SIGN_BIT);
-}
+#define SSN_lt(a,b)		\
+	(typecheck(__u16, a) && \
+	 typecheck(__u16, b) && \
+	 ((__s16)((a) - (b)) < 0))
 
 /*
  * ADDIP 3.1.1
  * The valid range of Serial Number is from 0 to 4294967295 (2**32 - 1). Serial
  * Numbers wrap back to 0 after reaching 4294967295.
  */
-enum {
-	ADDIP_SERIAL_SIGN_BIT = (1<<31)
-};
-
-static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
-{
-	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
-}
+#define ADDIP_SERIAL_gte(a,b)	\
+	(typecheck(__u32, a) && \
+	 typecheck(__u32, b) && \
+	 ((__s32)((b) - (a)) <= 0))
 
 /* Check VTAG of the packet matches the sender's own tag. */
 static inline int
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 72e54a416af6..6612d52599ae 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -1747,7 +1747,7 @@ static int sctp_acked(struct sctp_sackhdr *sack, __u32 tsn)
 {
 	int i;
 	sctp_sack_variable_t *frags;
-	__u16 gap;
+	__u16 gap, blocks;
 	__u32 ctsn = ntohl(sack->cum_tsn_ack);
 
 	if (TSN_lte(tsn, ctsn))
@@ -1767,9 +1767,10 @@ static int sctp_acked(struct sctp_sackhdr *sack, __u32 tsn)
 
 	frags = sack->variable;
 	gap = tsn - ctsn;
-	for (i = 0; i < ntohs(sack->num_gap_ack_blocks); ++i) {
-		if (TSN_lte(ntohs(frags[i].gab.start), gap) &&
-		    TSN_lte(gap, ntohs(frags[i].gab.end)))
+	blocks = ntohs(sack->num_gap_ack_blocks);
+	for (i = 0; i < blocks; ++i) {
+		if (TSN_lte((__u32)ntohs(frags[i].gab.start), (__u32)gap) &&
+		    TSN_lte((__u32)gap, (__u32)ntohs(frags[i].gab.end)))
 			goto pass;
 	}
 

RE: [PATCH net] sctp: fix SSN comparision

[David Laight]

From: Marcelo Ricardo Leitner
> Sent: 16 September 2016 14:45
> To: David Laight
> Cc: 'Neil Horman'; linux-sctp@vger.kernel.org
> Subject: Re: [PATCH net] sctp: fix SSN comparision
> 
> On Fri, Sep 16, 2016 at 10:40:47AM -0300, Marcelo Ricardo Leitner wrote:
> > On Fri, Sep 16, 2016 at 01:33:47PM +0000, David Laight wrote:
> > > > > -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> > > > > +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
> > > > >  {
> > > > >  	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
> > > > >  }
> > >
> > > Does gcc manage to compile that to (s32)((t) - (s)) <= 0 ?
> >
> > I have no idea but I have a patch in the works for updating it to this
> > form, to be more like time_after(). Will probably post it by next week.
> 
> Just for reference, this should be it. Not realy tested yet.
> 
> commit b52575533dfefa1908c7bbe35bde0fa8359dd1a3
> Author: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Date:   Tue Aug 9 14:45:35 2016 -0300
> 
>     sctp: improve how SSN, TSN and ASCONF serial are compared
> 
>     Make it similar to time_before() macros instead.
> 
>     This patch also removes SSN_lte as it is not used.
...
> +#define TSN_lte(a,b)	\
> +	(typecheck(__u32, a) && \
> +	 typecheck(__u32, b) && \
> +	 ((__s32)((a) - (b)) <= 0))
...
> +	for (i = 0; i < blocks; ++i) {
> +		if (TSN_lte((__u32)ntohs(frags[i].gab.start), (__u32)gap) &&
> +		    TSN_lte((__u32)gap, (__u32)ntohs(frags[i].gab.end)))

Those casts look decidedly dubious, messy at best.
I'm pretty sure you're not managing modulo 2^16 arithmetic either.
I think you want:
#define TSN_lte(a, b) ((__s16)((a) - (b)) <= 0)
If a and b are 16bit the subtract is always well defined.

	David

Re: [PATCH net] sctp: fix SSN comparision

['Marcelo Ricardo Leitner']

On Fri, Sep 16, 2016 at 02:17:15PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 16 September 2016 14:45
> > To: David Laight
> > Cc: 'Neil Horman'; linux-sctp@vger.kernel.org
> > Subject: Re: [PATCH net] sctp: fix SSN comparision
> > 
> > On Fri, Sep 16, 2016 at 10:40:47AM -0300, Marcelo Ricardo Leitner wrote:
> > > On Fri, Sep 16, 2016 at 01:33:47PM +0000, David Laight wrote:
> > > > > > -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> > > > > > +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
> > > > > >  {
> > > > > >  	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
> > > > > >  }
> > > >
> > > > Does gcc manage to compile that to (s32)((t) - (s)) <= 0 ?
> > >
> > > I have no idea but I have a patch in the works for updating it to this
> > > form, to be more like time_after(). Will probably post it by next week.
> > 
> > Just for reference, this should be it. Not realy tested yet.
> > 
> > commit b52575533dfefa1908c7bbe35bde0fa8359dd1a3
> > Author: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > Date:   Tue Aug 9 14:45:35 2016 -0300
> > 
> >     sctp: improve how SSN, TSN and ASCONF serial are compared
> > 
> >     Make it similar to time_before() macros instead.
> > 
> >     This patch also removes SSN_lte as it is not used.
> ...
> > +#define TSN_lte(a,b)	\
> > +	(typecheck(__u32, a) && \
> > +	 typecheck(__u32, b) && \
> > +	 ((__s32)((a) - (b)) <= 0))
> ...
> > +	for (i = 0; i < blocks; ++i) {
> > +		if (TSN_lte((__u32)ntohs(frags[i].gab.start), (__u32)gap) &&
> > +		    TSN_lte((__u32)gap, (__u32)ntohs(frags[i].gab.end)))
> 
> Those casts look decidedly dubious, messy at best.
> I'm pretty sure you're not managing modulo 2^16 arithmetic either.

Agreed.
Just to highlight, this is an issue today already, as they are promoted
to u32 on the function call.

> I think you want:
> #define TSN_lte(a, b) ((__s16)((a) - (b)) <= 0)
> If a and b are 16bit the subtract is always well defined.

No because it's also used in other places and with actual 32bit vars.
This was the only place that required such casts.

Thanks,
Marcelo

Re: [PATCH net] sctp: fix SSN comparision

['Marcelo Ricardo Leitner']

On Fri, Sep 16, 2016 at 11:46:24AM -0300, 'Marcelo Ricardo Leitner' wrote:
> On Fri, Sep 16, 2016 at 02:17:15PM +0000, David Laight wrote:
> > From: Marcelo Ricardo Leitner
> > > Sent: 16 September 2016 14:45
> > > To: David Laight
> > > Cc: 'Neil Horman'; linux-sctp@vger.kernel.org
> > > Subject: Re: [PATCH net] sctp: fix SSN comparision
> > > 
> > > On Fri, Sep 16, 2016 at 10:40:47AM -0300, Marcelo Ricardo Leitner wrote:
> > > > On Fri, Sep 16, 2016 at 01:33:47PM +0000, David Laight wrote:
> > > > > > > -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> > > > > > > +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
> > > > > > >  {
> > > > > > >  	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
> > > > > > >  }
> > > > >
> > > > > Does gcc manage to compile that to (s32)((t) - (s)) <= 0 ?
> > > >
> > > > I have no idea but I have a patch in the works for updating it to this
> > > > form, to be more like time_after(). Will probably post it by next week.
> > > 
> > > Just for reference, this should be it. Not realy tested yet.
> > > 
> > > commit b52575533dfefa1908c7bbe35bde0fa8359dd1a3
> > > Author: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > > Date:   Tue Aug 9 14:45:35 2016 -0300
> > > 
> > >     sctp: improve how SSN, TSN and ASCONF serial are compared
> > > 
> > >     Make it similar to time_before() macros instead.
> > > 
> > >     This patch also removes SSN_lte as it is not used.
> > ...
> > > +#define TSN_lte(a,b)	\
> > > +	(typecheck(__u32, a) && \
> > > +	 typecheck(__u32, b) && \
> > > +	 ((__s32)((a) - (b)) <= 0))
> > ...
> > > +	for (i = 0; i < blocks; ++i) {
> > > +		if (TSN_lte((__u32)ntohs(frags[i].gab.start), (__u32)gap) &&
> > > +		    TSN_lte((__u32)gap, (__u32)ntohs(frags[i].gab.end)))
> > 
> > Those casts look decidedly dubious, messy at best.
> > I'm pretty sure you're not managing modulo 2^16 arithmetic either.
> 
> Agreed.
> Just to highlight, this is an issue today already, as they are promoted
> to u32 on the function call.
> 
> > I think you want:
> > #define TSN_lte(a, b) ((__s16)((a) - (b)) <= 0)
> > If a and b are 16bit the subtract is always well defined.
> 
> No because it's also used in other places and with actual 32bit vars.
> This was the only place that required such casts.

That's why the typecheck() is good in this case, I think.

  Marcelo

RE: [PATCH net] sctp: fix SSN comparision

[David Laight]

From: 'Marcelo Ricardo Leitner'
> Sent: 16 September 2016 15:53
> On Fri, Sep 16, 2016 at 11:46:24AM -0300, 'Marcelo Ricardo Leitner' wrote:
> > On Fri, Sep 16, 2016 at 02:17:15PM +0000, David Laight wrote:
> > > From: Marcelo Ricardo Leitner
> > > > Sent: 16 September 2016 14:45
> > > > To: David Laight
> > > > Cc: 'Neil Horman'; linux-sctp@vger.kernel.org
> > > > Subject: Re: [PATCH net] sctp: fix SSN comparision
> > > >
> > > > On Fri, Sep 16, 2016 at 10:40:47AM -0300, Marcelo Ricardo Leitner wrote:
> > > > > On Fri, Sep 16, 2016 at 01:33:47PM +0000, David Laight wrote:
> > > > > > > > -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> > > > > > > > +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
> > > > > > > >  {
> > > > > > > >  	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
> > > > > > > >  }
> > > > > >
> > > > > > Does gcc manage to compile that to (s32)((t) - (s)) <= 0 ?
> > > > >
> > > > > I have no idea but I have a patch in the works for updating it to this
> > > > > form, to be more like time_after(). Will probably post it by next week.
> > > >
> > > > Just for reference, this should be it. Not realy tested yet.
> > > >
> > > > commit b52575533dfefa1908c7bbe35bde0fa8359dd1a3
> > > > Author: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > > > Date:   Tue Aug 9 14:45:35 2016 -0300
> > > >
> > > >     sctp: improve how SSN, TSN and ASCONF serial are compared
> > > >
> > > >     Make it similar to time_before() macros instead.
> > > >
> > > >     This patch also removes SSN_lte as it is not used.
> > > ...
> > > > +#define TSN_lte(a,b)	\
> > > > +	(typecheck(__u32, a) && \
> > > > +	 typecheck(__u32, b) && \
> > > > +	 ((__s32)((a) - (b)) <= 0))
> > > ...
> > > > +	for (i = 0; i < blocks; ++i) {
> > > > +		if (TSN_lte((__u32)ntohs(frags[i].gab.start), (__u32)gap) &&
> > > > +		    TSN_lte((__u32)gap, (__u32)ntohs(frags[i].gab.end)))
> > >
> > > Those casts look decidedly dubious, messy at best.
> > > I'm pretty sure you're not managing modulo 2^16 arithmetic either.
> >
> > Agreed.
> > Just to highlight, this is an issue today already, as they are promoted
> > to u32 on the function call.
> >
> > > I think you want:
> > > #define TSN_lte(a, b) ((__s16)((a) - (b)) <= 0)
> > > If a and b are 16bit the subtract is always well defined.
> >
> > No because it's also used in other places and with actual 32bit vars.
> > This was the only place that required such casts.
> 
> That's why the typecheck() is good in this case, I think.

Doing modulo 2^32 arithmetic on 16 bit data doesn't make sense at all.
If these values are 16bit truncations of a 32bit value you still need to
do modulo 2^16 sums.
So the typecheck() is probably good, but the casts are bad.

However holding values in 'word' sized local variables will almost
certainly generate better code than using u16 - especially if any
arithmetic is done on the values.
So the typecheck() may be troublesome elsewhere!

	David

Re: [PATCH net] sctp: fix SSN comparision

['Marcelo Ricardo Leitner']

On Fri, Sep 16, 2016 at 03:13:58PM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 16 September 2016 15:53
> > On Fri, Sep 16, 2016 at 11:46:24AM -0300, 'Marcelo Ricardo Leitner' wrote:
> > > On Fri, Sep 16, 2016 at 02:17:15PM +0000, David Laight wrote:
> > > > From: Marcelo Ricardo Leitner
> > > > > Sent: 16 September 2016 14:45
> > > > > To: David Laight
> > > > > Cc: 'Neil Horman'; linux-sctp@vger.kernel.org
> > > > > Subject: Re: [PATCH net] sctp: fix SSN comparision
> > > > >
> > > > > On Fri, Sep 16, 2016 at 10:40:47AM -0300, Marcelo Ricardo Leitner wrote:
> > > > > > On Fri, Sep 16, 2016 at 01:33:47PM +0000, David Laight wrote:
> > > > > > > > > -static inline int ADDIP_SERIAL_gte(__u16 s, __u16 t)
> > > > > > > > > +static inline int ADDIP_SERIAL_gte(__u32 s, __u32 t)
> > > > > > > > >  {
> > > > > > > > >  	return ((s) = (t)) || (((t) - (s)) & ADDIP_SERIAL_SIGN_BIT);
> > > > > > > > >  }
> > > > > > >
> > > > > > > Does gcc manage to compile that to (s32)((t) - (s)) <= 0 ?
> > > > > >
> > > > > > I have no idea but I have a patch in the works for updating it to this
> > > > > > form, to be more like time_after(). Will probably post it by next week.
> > > > >
> > > > > Just for reference, this should be it. Not realy tested yet.
> > > > >
> > > > > commit b52575533dfefa1908c7bbe35bde0fa8359dd1a3
> > > > > Author: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > > > > Date:   Tue Aug 9 14:45:35 2016 -0300
> > > > >
> > > > >     sctp: improve how SSN, TSN and ASCONF serial are compared
> > > > >
> > > > >     Make it similar to time_before() macros instead.
> > > > >
> > > > >     This patch also removes SSN_lte as it is not used.
> > > > ...
> > > > > +#define TSN_lte(a,b)	\
> > > > > +	(typecheck(__u32, a) && \
> > > > > +	 typecheck(__u32, b) && \
> > > > > +	 ((__s32)((a) - (b)) <= 0))
> > > > ...
> > > > > +	for (i = 0; i < blocks; ++i) {
> > > > > +		if (TSN_lte((__u32)ntohs(frags[i].gab.start), (__u32)gap) &&
> > > > > +		    TSN_lte((__u32)gap, (__u32)ntohs(frags[i].gab.end)))
> > > >
> > > > Those casts look decidedly dubious, messy at best.
> > > > I'm pretty sure you're not managing modulo 2^16 arithmetic either.
> > >
> > > Agreed.
> > > Just to highlight, this is an issue today already, as they are promoted
> > > to u32 on the function call.
> > >
> > > > I think you want:
> > > > #define TSN_lte(a, b) ((__s16)((a) - (b)) <= 0)
> > > > If a and b are 16bit the subtract is always well defined.
> > >
> > > No because it's also used in other places and with actual 32bit vars.
> > > This was the only place that required such casts.
> > 
> > That's why the typecheck() is good in this case, I think.
> 
> Doing modulo 2^32 arithmetic on 16 bit data doesn't make sense at all.
> If these values are 16bit truncations of a 32bit value you still need to
> do modulo 2^16 sums.
> So the typecheck() is probably good, but the casts are bad.

According to the RFC these should be small offsets to the ctsn (u32)
sent together with the SACK chunk. I'm thinking it's best/easier if we
don't do the gap calc in there but just compute the final offsets by
summing ctsn and the gap ack limits, and work it all on 32bit.

As in:
@@ -1766,10 +1766,10 @@ static int sctp_acked(struct sctp_sackhdr *sack, __u32 tsn)
         */
 
        frags = sack->variable;
-       gap = tsn - ctsn;
-       for (i = 0; i < ntohs(sack->num_gap_ack_blocks); ++i) {
-               if (TSN_lte(ntohs(frags[i].gab.start), gap) &&
-                   TSN_lte(gap, ntohs(frags[i].gab.end)))
+       blocks = ntohs(sack->num_gap_ack_blocks);
+       for (i = 0; i < blocks; ++i) {
+               if (TSN_lte(ctsn + ntohs(frags[i].gab.start), tsn) &&
+                   TSN_lte(tsn, ctsn + ntohs(frags[i].gab.end)))
                        goto pass;
        }

I don't think that doing 2 sums inside the for() will result in any
noticeable performance degradation and will keep all TSN handling in
32bit arithmetics.

wdyt?

> However holding values in 'word' sized local variables will almost
> certainly generate better code than using u16 - especially if any
> arithmetic is done on the values.
> So the typecheck() may be troublesome elsewhere!

Re: [PATCH net] sctp: fix SSN comparision

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Thu, 15 Sep 2016 15:02:38 -0300

> This function actually operates on u32 yet its paramteres were declared
> as u16, causing integer truncation upon calling.
> 
> Note in patch context that ADDIP_SERIAL_SIGN_BIT is already 32 bits.
> 
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Applied.

Re: [PATCH net] sctp: fix SSN comparision

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Thu, 15 Sep 2016 15:02:38 -0300

> This function actually operates on u32 yet its paramteres were declared
> as u16, causing integer truncation upon calling.
> 
> Note in patch context that ADDIP_SERIAL_SIGN_BIT is already 32 bits.
> 
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Applied.

RE: [PATCH net] sctp: fix SSN comparision

[David Laight]

From: 'Marcelo Ricardo Leitner'
> Sent: 16 September 2016 18:22
...
>         frags = sack->variable;
> -       gap = tsn - ctsn;
> -       for (i = 0; i < ntohs(sack->num_gap_ack_blocks); ++i) {
> -               if (TSN_lte(ntohs(frags[i].gab.start), gap) &&
> -                   TSN_lte(gap, ntohs(frags[i].gab.end)))
> +       blocks = ntohs(sack->num_gap_ack_blocks);
> +       for (i = 0; i < blocks; ++i) {
> +               if (TSN_lte(ctsn + ntohs(frags[i].gab.start), tsn) &&
> +                   TSN_lte(tsn, ctsn + ntohs(frags[i].gab.end)))
>                         goto pass;

I think I now understand what all the values are!
The correct test is just:
			if (gap > ntohs(frags[i].gab.start) &&
			    gap <= ntohs(frags[i].gab.end))
(I think the ends are right).
gab.start/end are 16bit unsigned offsets from ctsn.
All the module arithmetic is sorted out when 'tsn' is converted to an
offset from ctsn. 'gap' is a very bad name for the tsn_offset.

Looks like gap.start/end should be saved in host order though.

	David

Re: [PATCH net] sctp: fix SSN comparision

['Marcelo Ricardo Leitner']

On Mon, Sep 19, 2016 at 10:41:34AM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 16 September 2016 18:22
> ...
> >         frags = sack->variable;
> > -       gap = tsn - ctsn;
> > -       for (i = 0; i < ntohs(sack->num_gap_ack_blocks); ++i) {
> > -               if (TSN_lte(ntohs(frags[i].gab.start), gap) &&
> > -                   TSN_lte(gap, ntohs(frags[i].gab.end)))
> > +       blocks = ntohs(sack->num_gap_ack_blocks);
> > +       for (i = 0; i < blocks; ++i) {
> > +               if (TSN_lte(ctsn + ntohs(frags[i].gab.start), tsn) &&
> > +                   TSN_lte(tsn, ctsn + ntohs(frags[i].gab.end)))
> >                         goto pass;
> 
> I think I now understand what all the values are!
> The correct test is just:
> 			if (gap > ntohs(frags[i].gab.start) &&
> 			    gap <= ntohs(frags[i].gab.end))
> (I think the ends are right).
> gab.start/end are 16bit unsigned offsets from ctsn.
> All the module arithmetic is sorted out when 'tsn' is converted to an
> offset from ctsn. 'gap' is a very bad name for the tsn_offset.

Yep! I like this approach, thanks.
And in case tsn is actually smaller than ctsn in this code, than that's
another bug actually, so I think your suggestion covers all the cases.

Though the ends are both inclusive.
Handy https://tools.ietf.org/html/rfc4960#section-3.3.4 if you want.

> 
> Looks like gap.start/end should be saved in host order though.

I lost you here. gab.start/end you mean? Saved?

  Marcelo

Re: [PATCH net] sctp: fix SSN comparision

['Marcelo Ricardo Leitner']

On Mon, Sep 19, 2016 at 01:02:55PM -0300, 'Marcelo Ricardo Leitner' wrote:
> On Mon, Sep 19, 2016 at 10:41:34AM +0000, David Laight wrote:
> > From: 'Marcelo Ricardo Leitner'
> > > Sent: 16 September 2016 18:22
> > ...
> > >         frags = sack->variable;
> > > -       gap = tsn - ctsn;
> > > -       for (i = 0; i < ntohs(sack->num_gap_ack_blocks); ++i) {
> > > -               if (TSN_lte(ntohs(frags[i].gab.start), gap) &&
> > > -                   TSN_lte(gap, ntohs(frags[i].gab.end)))
> > > +       blocks = ntohs(sack->num_gap_ack_blocks);
> > > +       for (i = 0; i < blocks; ++i) {
> > > +               if (TSN_lte(ctsn + ntohs(frags[i].gab.start), tsn) &&
> > > +                   TSN_lte(tsn, ctsn + ntohs(frags[i].gab.end)))
> > >                         goto pass;
> > 
> > I think I now understand what all the values are!
> > The correct test is just:
> > 			if (gap > ntohs(frags[i].gab.start) &&
> > 			    gap <= ntohs(frags[i].gab.end))
> > (I think the ends are right).
> > gab.start/end are 16bit unsigned offsets from ctsn.
> > All the module arithmetic is sorted out when 'tsn' is converted to an
> > offset from ctsn. 'gap' is a very bad name for the tsn_offset.
> 
> Yep! I like this approach, thanks.
> And in case tsn is actually smaller than ctsn in this code, than that's
> another bug actually, so I think your suggestion covers all the cases.

Confirming, this situation is verified earlier in the same function, all
good.

> 
> Though the ends are both inclusive.
> Handy https://tools.ietf.org/html/rfc4960#section-3.3.4 if you want.
> 
> > 
> > Looks like gap.start/end should be saved in host order though.
> 
> I lost you here. gab.start/end you mean? Saved?
> 
>   Marcelo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>