[PATCH] sctp: chunkmap size is too large

[PATCH] sctp: chunkmap size is too large

[Olivier MATZ]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

Hi all,

I think that the chunkmap field in struct xt_sctp_info is
too large: we have 256 chunk types and it's a bitfield,
so the table should be 256 bits (8 uint32_t) instead
of 64 uint32_t.

I updated the size of the table, and I did some cosmetic
changes in SCTP_CHUNKMAP_* macros (use a mask instead of
a modulo).

  netfilter/xt_sctp.h       |   30 ++++++++++++++++--------------
  netfilter_ipv4/ipt_sctp.h |   30 ++++++++++++++++--------------
  2 files changed, 32 insertions(+), 28 deletions(-)

Can someone have a look at it ? Please CC me for any replies
as I'm not on the list.

Thanks,
Olivier

[-- Attachment #2: linux_sctp_chunkmap_too_large.diff --]
[-- Type: text/x-patch, Size: 4063 bytes --]

diff -r ce4087e26684 include/linux/netfilter/xt_sctp.h
--- a/include/linux/netfilter/xt_sctp.h	Wed Jan 28 20:01:08 2009 +0000
+++ b/include/linux/netfilter/xt_sctp.h	Fri Jan 30 16:25:55 2009 +0100
@@ -15,11 +15,13 @@
 
 #define XT_NUM_SCTP_FLAGS	4
 
+#define sizeof_bits(type) (sizeof(type) * 8)
+
 struct xt_sctp_info {
 	u_int16_t dpts[2];  /* Min, Max */
 	u_int16_t spts[2];  /* Min, Max */
 
-	u_int32_t chunkmap[256 / sizeof (u_int32_t)];  /* Bit mask of chunks to be matched according to RFC 2960 */
+	u_int32_t chunkmap[256 / sizeof_bits(u_int32_t)];  /* Bit mask of chunks to be matched according to RFC 2960 */
 
 #define SCTP_CHUNK_MATCH_ANY   0x01  /* Match if any of the chunk types are present */
 #define SCTP_CHUNK_MATCH_ALL   0x02  /* Match if all of the chunk types are present */
@@ -33,24 +35,24 @@
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
+#define SCTP_MODULO(chunktype, type) (chunktype & (sizeof_bits(type)-1))
 
-#define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
-	do { 						\
-		(chunkmap)[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+#define SCTP_CHUNKMAP_SET(chunkmap, chunktype)				\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] |=		\
+			1 << SCTP_MODULO(chunktype, u_int32_t);		\
 	} while (0)
 
-#define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
-	do {							\
-		(chunkmap)[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+#define SCTP_CHUNKMAP_CLEAR(chunkmap, chunktype)		 	\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] &=		\
+			~(1 << SCTP_MODULO(chunktype, u_int32_t));	\
 	} while (0)
 
-#define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
-({								\
-	((chunkmap)[type / bytes (u_int32_t)] & 		\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+#define SCTP_CHUNKMAP_IS_SET(chunkmap, chunktype) 			\
+({									\
+	(chunkmap[chunktype / sizeof_bits(u_int32_t)] &			\
+	 (1 << SCTP_MODULO(chunktype, u_int32_t))) ? 1 : 0;		\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) \
diff -r ce4087e26684 include/linux/netfilter_ipv4/ipt_sctp.h
--- a/include/linux/netfilter_ipv4/ipt_sctp.h	Wed Jan 28 20:01:08 2009 +0000
+++ b/include/linux/netfilter_ipv4/ipt_sctp.h	Fri Jan 30 16:25:55 2009 +0100
@@ -16,11 +16,13 @@
 
 #define IPT_NUM_SCTP_FLAGS	4
 
+#define sizeof_bits(type) (sizeof(type) * 8)
+
 struct ipt_sctp_info {
 	u_int16_t dpts[2];  /* Min, Max */
 	u_int16_t spts[2];  /* Min, Max */
 
-	u_int32_t chunkmap[256 / sizeof (u_int32_t)];  /* Bit mask of chunks to be matched according to RFC 2960 */
+	u_int32_t chunkmap[256 / sizeof_bits(u_int32_t)];  /* Bit mask of chunks to be matched according to RFC 2960 */
 
 #define SCTP_CHUNK_MATCH_ANY   0x01  /* Match if any of the chunk types are present */
 #define SCTP_CHUNK_MATCH_ALL   0x02  /* Match if all of the chunk types are present */
@@ -34,24 +36,24 @@
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
+#define SCTP_MODULO(chunktype, type) (chunktype & (sizeof_bits(type)-1))
 
-#define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
-	do { 						\
-		chunkmap[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+#define SCTP_CHUNKMAP_SET(chunkmap, chunktype)				\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] |=		\
+			1 << SCTP_MODULO(chunktype, u_int32_t);		\
 	} while (0)
 
-#define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
-	do {							\
-		chunkmap[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+#define SCTP_CHUNKMAP_CLEAR(chunkmap, chunktype)		 	\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] &=		\
+			~(1 << SCTP_MODULO(chunktype, u_int32_t));	\
 	} while (0)
 
-#define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
-({								\
-	(chunkmap[type / bytes (u_int32_t)] & 			\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+#define SCTP_CHUNKMAP_IS_SET(chunkmap, chunktype) 			\
+({									\
+	(chunkmap[chunktype / sizeof_bits(u_int32_t)] &			\
+	 (1 << SCTP_MODULO(chunktype, u_int32_t))) ? 1 : 0;		\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) 				\

Re: [PATCH] sctp: chunkmap size is too large

[David Miller]

From: Olivier MATZ <olivier.matz@6wind.com>
Date: Fri, 30 Jan 2009 16:43:07 +0100

> I think that the chunkmap field in struct xt_sctp_info is
> too large: we have 256 chunk types and it's a bitfield,
> so the table should be 256 bits (8 uint32_t) instead
> of 64 uint32_t.
> 
> I updated the size of the table, and I did some cosmetic
> changes in SCTP_CHUNKMAP_* macros (use a mask instead of
> a modulo).
> 
>   netfilter/xt_sctp.h       |   30 ++++++++++++++++--------------
>   netfilter_ipv4/ipt_sctp.h |   30 ++++++++++++++++--------------
>   2 files changed, 32 insertions(+), 28 deletions(-)
> 
> Can someone have a look at it ? Please CC me for any replies
> as I'm not on the list.

Patrick, I assume you got this?

Re: [PATCH] sctp: chunkmap size is too large

[Olivier MATZ]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

Hi,

A file was missing in the previous patch. I think that
the size of chunkmapcopy in xt_sctp.c should also be
fixed.

I attached the new patch.

  include/linux/netfilter/xt_sctp.h       |   30 
++++++++++++++++--------------
  include/linux/netfilter_ipv4/ipt_sctp.h |   30 
++++++++++++++++--------------
  net/netfilter/xt_sctp.c                 |    2 +-
  3 files changed, 33 insertions(+), 29 deletions(-)

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Thanks,
Olivier


[-- Attachment #2: linux_sctp_chunkmap_too_large_b.diff --]
[-- Type: text/x-patch, Size: 4524 bytes --]

diff -r 94166a3a38bd include/linux/netfilter/xt_sctp.h
--- a/include/linux/netfilter/xt_sctp.h	Sat Jan 31 15:56:23 2009 -0800
+++ b/include/linux/netfilter/xt_sctp.h	Mon Feb 02 10:18:51 2009 +0100
@@ -15,11 +15,13 @@
 
 #define XT_NUM_SCTP_FLAGS	4
 
+#define sizeof_bits(type) (sizeof(type) * 8)
+
 struct xt_sctp_info {
 	u_int16_t dpts[2];  /* Min, Max */
 	u_int16_t spts[2];  /* Min, Max */
 
-	u_int32_t chunkmap[256 / sizeof (u_int32_t)];  /* Bit mask of chunks to be matched according to RFC 2960 */
+	u_int32_t chunkmap[256 / sizeof_bits(u_int32_t)];  /* Bit mask of chunks to be matched according to RFC 2960 */
 
 #define SCTP_CHUNK_MATCH_ANY   0x01  /* Match if any of the chunk types are present */
 #define SCTP_CHUNK_MATCH_ALL   0x02  /* Match if all of the chunk types are present */
@@ -33,24 +35,24 @@
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
+#define SCTP_MODULO(chunktype, type) (chunktype & (sizeof_bits(type)-1))
 
-#define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
-	do { 						\
-		(chunkmap)[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+#define SCTP_CHUNKMAP_SET(chunkmap, chunktype)				\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] |=		\
+			1 << SCTP_MODULO(chunktype, u_int32_t);		\
 	} while (0)
 
-#define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
-	do {							\
-		(chunkmap)[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+#define SCTP_CHUNKMAP_CLEAR(chunkmap, chunktype)		 	\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] &=		\
+			~(1 << SCTP_MODULO(chunktype, u_int32_t));	\
 	} while (0)
 
-#define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
-({								\
-	((chunkmap)[type / bytes (u_int32_t)] & 		\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+#define SCTP_CHUNKMAP_IS_SET(chunkmap, chunktype) 			\
+({									\
+	(chunkmap[chunktype / sizeof_bits(u_int32_t)] &			\
+	 (1 << SCTP_MODULO(chunktype, u_int32_t))) ? 1 : 0;		\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) \
diff -r 94166a3a38bd include/linux/netfilter_ipv4/ipt_sctp.h
--- a/include/linux/netfilter_ipv4/ipt_sctp.h	Sat Jan 31 15:56:23 2009 -0800
+++ b/include/linux/netfilter_ipv4/ipt_sctp.h	Mon Feb 02 10:18:51 2009 +0100
@@ -16,11 +16,13 @@
 
 #define IPT_NUM_SCTP_FLAGS	4
 
+#define sizeof_bits(type) (sizeof(type) * 8)
+
 struct ipt_sctp_info {
 	u_int16_t dpts[2];  /* Min, Max */
 	u_int16_t spts[2];  /* Min, Max */
 
-	u_int32_t chunkmap[256 / sizeof (u_int32_t)];  /* Bit mask of chunks to be matched according to RFC 2960 */
+	u_int32_t chunkmap[256 / sizeof_bits(u_int32_t)];  /* Bit mask of chunks to be matched according to RFC 2960 */
 
 #define SCTP_CHUNK_MATCH_ANY   0x01  /* Match if any of the chunk types are present */
 #define SCTP_CHUNK_MATCH_ALL   0x02  /* Match if all of the chunk types are present */
@@ -34,24 +36,24 @@
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
+#define SCTP_MODULO(chunktype, type) (chunktype & (sizeof_bits(type)-1))
 
-#define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
-	do { 						\
-		chunkmap[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+#define SCTP_CHUNKMAP_SET(chunkmap, chunktype)				\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] |=		\
+			1 << SCTP_MODULO(chunktype, u_int32_t);		\
 	} while (0)
 
-#define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
-	do {							\
-		chunkmap[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+#define SCTP_CHUNKMAP_CLEAR(chunkmap, chunktype)		 	\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] &=		\
+			~(1 << SCTP_MODULO(chunktype, u_int32_t));	\
 	} while (0)
 
-#define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
-({								\
-	(chunkmap[type / bytes (u_int32_t)] & 			\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+#define SCTP_CHUNKMAP_IS_SET(chunkmap, chunktype) 			\
+({									\
+	(chunkmap[chunktype / sizeof_bits(u_int32_t)] &			\
+	 (1 << SCTP_MODULO(chunktype, u_int32_t))) ? 1 : 0;		\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) 				\
diff -r 94166a3a38bd net/netfilter/xt_sctp.c
--- a/net/netfilter/xt_sctp.c	Sat Jan 31 15:56:23 2009 -0800
+++ b/net/netfilter/xt_sctp.c	Mon Feb 02 10:18:51 2009 +0100
@@ -45,7 +45,7 @@
 	     const struct xt_sctp_info *info,
 	     bool *hotdrop)
 {
-	u_int32_t chunkmapcopy[256 / sizeof (u_int32_t)];
+	u_int32_t chunkmapcopy[256 / sizeof_bits(u_int32_t)];
 	const sctp_chunkhdr_t *sch;
 	sctp_chunkhdr_t _sch;
 	int chunk_match_type = info->chunk_match_type;

Re: [PATCH] sctp: chunkmap size is too large

[Jan Engelhardt]

On Sunday 2009-02-01 10:14, David Miller wrote:
>> I think that the chunkmap field in struct xt_sctp_info is
>> too large: we have 256 chunk types and it's a bitfield,
>> so the table should be 256 bits (8 uint32_t) instead
>> of 64 uint32_t.
>> 
>> I updated the size of the table, and I did some cosmetic
>> changes in SCTP_CHUNKMAP_* macros (use a mask instead of
>> a modulo).
>> 
>>   netfilter/xt_sctp.h       |   30 ++++++++++++++++--------------
>>   netfilter_ipv4/ipt_sctp.h |   30 ++++++++++++++++--------------
>>   2 files changed, 32 insertions(+), 28 deletions(-)
>> 
>> Can someone have a look at it ? Please CC me for any replies
>> as I'm not on the list.
>
>Patrick, I assume you got this?

At least I got this - and the change would have to be nak'ed because
it changes the userspace<->kernel binary stream. If you do such a
change, a new match revision is required.

Re: [PATCH] sctp: chunkmap size is too large

[Patrick McHardy]

David Miller wrote:
> From: Olivier MATZ <olivier.matz@6wind.com>
> Date: Fri, 30 Jan 2009 16:43:07 +0100
> 
>> I think that the chunkmap field in struct xt_sctp_info is
>> too large: we have 256 chunk types and it's a bitfield,
>> so the table should be 256 bits (8 uint32_t) instead
>> of 64 uint32_t.
>>
>> I updated the size of the table, and I did some cosmetic
>> changes in SCTP_CHUNKMAP_* macros (use a mask instead of
>> a modulo).
>>
>>   netfilter/xt_sctp.h       |   30 ++++++++++++++++--------------
>>   netfilter_ipv4/ipt_sctp.h |   30 ++++++++++++++++--------------
>>   2 files changed, 32 insertions(+), 28 deletions(-)
>>
>> Can someone have a look at it ? Please CC me for any replies
>> as I'm not on the list.
> 
> Patrick, I assume you got this?

Sorry for the delay. As Jan already noticed, it changes the ABI,
so we change it without adding a new revision. And I don't think
a few bytes of memory savings justify that.