diff mbox

[-next,v3,1/3] syncookies: avoid magic values and document which-bit-is-what-option

Message ID 1415019720-17106-2-git-send-email-fw@strlen.de
State Superseded, archived
Headers show

Commit Message

Florian Westphal Nov. 3, 2014, 1:01 p.m. UTC 
Was a bit more difficult to read than needed due to magic shifts;
add defines and document the used encoding scheme.

Joint work with Daniel Borkmann.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 This patch was not part of earlier versions of the set.

 net/ipv4/syncookies.c | 50 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 15 deletions(-)

Comments

David Laight Nov. 3, 2014, 2:24 p.m. UTC | #1 
From: Florian Westphal
> Was a bit more difficult to read than needed due to magic shifts;
> add defines and document the used encoding scheme.
> 
> Joint work with Daniel Borkmann.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  This patch was not part of earlier versions of the set.
> 
>  net/ipv4/syncookies.c | 50 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index 4ac7bca..c3792c0 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -19,10 +19,6 @@
>  #include <net/tcp.h>
>  #include <net/route.h>
> 
> -/* Timestamps: lowest bits store TCP options */
> -#define TSBITS 6
> -#define TSMASK (((__u32)1 << TSBITS) - 1)
> -
>  extern int sysctl_tcp_syncookies;
> 
>  static u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS] __read_mostly;
> @@ -30,6 +26,30 @@ static u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS] __read_mostly;
>  #define COOKIEBITS 24	/* Upper bits store count */
>  #define COOKIEMASK (((__u32)1 << COOKIEBITS) - 1)
> 
> +/* TCP Timestamp: 6 lowest bits of timestamp sent in the cookie SYN-ACK
> + * stores TCP options:
> + *
> + * MSB                               LSB
> + * | 31 ...   6 |  5  |  4   | 3 2 1 0 |
> + * |  Timestamp | ECN | SACK | WScale  |
> + *
> + * When we receive a valid cookie-ACK, we look at the echoed tsval (if
> + * any) to figure out which TCP options we should use for the rebuilt
> + * connection.
> + *
> + * A WScale setting of '0xf' (which is an invalid scaling value)
> + * means that original syn did not include the TCP window scaling option.
> + */
> +#define TS_OPT_WSCALE_MASK	0xf
> +#define TS_OPT_SACK		BIT(4)
> +#define TS_OPT_ECN		BIT(5)
> +/* There is no TS_OPT_TIMESTAMP:
> + * if ACK contains timestamp option, we already know it was
> + * requested/supported by the syn/synack exchange.
> + */
> +#define TSBITS	6
> +#define TSMASK	(((__u32)1 << TSBITS) - 1)

Personally I'd define all the values as hex constants instead of mixing
and matching the defines.

So probably just:
#define TS_OPT_WSCALE_MASK	0x0f
#define TS_OPT_SACK		0x10
#define TS_OPT_ECN		0x20
#define TSMASK                0x3f

	David





--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Borkmann Nov. 3, 2014, 2:33 p.m. UTC | #2 
On 11/03/2014 03:24 PM, David Laight wrote:
> From: Florian Westphal
>> Was a bit more difficult to read than needed due to magic shifts;
>> add defines and document the used encoding scheme.
>>
>> Joint work with Daniel Borkmann.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> Signed-off-by: Florian Westphal <fw@strlen.de>
>> ---
>>   This patch was not part of earlier versions of the set.
>>
>>   net/ipv4/syncookies.c | 50 +++++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 35 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
>> index 4ac7bca..c3792c0 100644
>> --- a/net/ipv4/syncookies.c
>> +++ b/net/ipv4/syncookies.c
>> @@ -19,10 +19,6 @@
>>   #include <net/tcp.h>
>>   #include <net/route.h>
>>
>> -/* Timestamps: lowest bits store TCP options */
>> -#define TSBITS 6
>> -#define TSMASK (((__u32)1 << TSBITS) - 1)
>> -
>>   extern int sysctl_tcp_syncookies;
>>
>>   static u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS] __read_mostly;
>> @@ -30,6 +26,30 @@ static u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS] __read_mostly;
>>   #define COOKIEBITS 24	/* Upper bits store count */
>>   #define COOKIEMASK (((__u32)1 << COOKIEBITS) - 1)
>>
>> +/* TCP Timestamp: 6 lowest bits of timestamp sent in the cookie SYN-ACK
>> + * stores TCP options:
>> + *
>> + * MSB                               LSB
>> + * | 31 ...   6 |  5  |  4   | 3 2 1 0 |
>> + * |  Timestamp | ECN | SACK | WScale  |
>> + *
>> + * When we receive a valid cookie-ACK, we look at the echoed tsval (if
>> + * any) to figure out which TCP options we should use for the rebuilt
>> + * connection.
>> + *
>> + * A WScale setting of '0xf' (which is an invalid scaling value)
>> + * means that original syn did not include the TCP window scaling option.
>> + */
>> +#define TS_OPT_WSCALE_MASK	0xf
>> +#define TS_OPT_SACK		BIT(4)
>> +#define TS_OPT_ECN		BIT(5)
>> +/* There is no TS_OPT_TIMESTAMP:
>> + * if ACK contains timestamp option, we already know it was
>> + * requested/supported by the syn/synack exchange.
>> + */
>> +#define TSBITS	6
>> +#define TSMASK	(((__u32)1 << TSBITS) - 1)
>
> Personally I'd define all the values as hex constants instead of mixing
> and matching the defines.
>
> So probably just:
> #define TS_OPT_WSCALE_MASK	0x0f
> #define TS_OPT_SACK		0x10
> #define TS_OPT_ECN		0x20
> #define TSMASK                0x3f

If you look at the above comment and then take a peek at the actual TS_OPT_*,
it is much easier to follow. Moreover, how is having TSMASK as 0x3f better?!
Currently, it is a constant calculated based upon TSBITS.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight Nov. 3, 2014, 2:41 p.m. UTC | #3 
From: Daniel Borkmann
> On 11/03/2014 03:24 PM, David Laight wrote:
> > From: Florian Westphal
> >> Was a bit more difficult to read than needed due to magic shifts;
> >> add defines and document the used encoding scheme.
> >>
> >> Joint work with Daniel Borkmann.
> >>
> >> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> >> Signed-off-by: Florian Westphal <fw@strlen.de>
> >> ---
> >>   This patch was not part of earlier versions of the set.
> >>
> >>   net/ipv4/syncookies.c | 50 +++++++++++++++++++++++++++++++++++---------------
> >>   1 file changed, 35 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> >> index 4ac7bca..c3792c0 100644
> >> --- a/net/ipv4/syncookies.c
> >> +++ b/net/ipv4/syncookies.c
> >> @@ -19,10 +19,6 @@
> >>   #include <net/tcp.h>
> >>   #include <net/route.h>
> >>
> >> -/* Timestamps: lowest bits store TCP options */
> >> -#define TSBITS 6
> >> -#define TSMASK (((__u32)1 << TSBITS) - 1)
> >> -
> >>   extern int sysctl_tcp_syncookies;
> >>
> >>   static u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS] __read_mostly;
> >> @@ -30,6 +26,30 @@ static u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS] __read_mostly;
> >>   #define COOKIEBITS 24	/* Upper bits store count */
> >>   #define COOKIEMASK (((__u32)1 << COOKIEBITS) - 1)
> >>
> >> +/* TCP Timestamp: 6 lowest bits of timestamp sent in the cookie SYN-ACK
> >> + * stores TCP options:
> >> + *
> >> + * MSB                               LSB
> >> + * | 31 ...   6 |  5  |  4   | 3 2 1 0 |
> >> + * |  Timestamp | ECN | SACK | WScale  |
> >> + *
> >> + * When we receive a valid cookie-ACK, we look at the echoed tsval (if
> >> + * any) to figure out which TCP options we should use for the rebuilt
> >> + * connection.
> >> + *
> >> + * A WScale setting of '0xf' (which is an invalid scaling value)
> >> + * means that original syn did not include the TCP window scaling option.
> >> + */
> >> +#define TS_OPT_WSCALE_MASK	0xf
> >> +#define TS_OPT_SACK		BIT(4)
> >> +#define TS_OPT_ECN		BIT(5)
> >> +/* There is no TS_OPT_TIMESTAMP:
> >> + * if ACK contains timestamp option, we already know it was
> >> + * requested/supported by the syn/synack exchange.
> >> + */
> >> +#define TSBITS	6
> >> +#define TSMASK	(((__u32)1 << TSBITS) - 1)
> >
> > Personally I'd define all the values as hex constants instead of mixing
> > and matching the defines.
> >
> > So probably just:
> > #define TS_OPT_WSCALE_MASK	0x0f
> > #define TS_OPT_SACK		0x10
> > #define TS_OPT_ECN		0x20
> > #define TSMASK                0x3f
> 
> If you look at the above comment and then take a peek at the actual TS_OPT_*,
> it is much easier to follow. Moreover, how is having TSMASK as 0x3f better?!
> Currently, it is a constant calculated based upon TSBITS.

TSMASK is also (TS_OPT_WSCALE_MASK | TS_OPT_SACK | TS_OPT_ECN) defining
the values in hex makes this even more clear.
Defining TSBITS from the mask would save the extra definition - which might
be easier done by replacing the shifts with multiply/divide by (TSMASK + 1)
(probably in a #define/inline function to make the code easier to read.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Borkmann Nov. 3, 2014, 3:27 p.m. UTC | #4 
On 11/03/2014 03:41 PM, David Laight wrote:
...
>>>> +/* TCP Timestamp: 6 lowest bits of timestamp sent in the cookie SYN-ACK
>>>> + * stores TCP options:
>>>> + *
>>>> + * MSB                               LSB
>>>> + * | 31 ...   6 |  5  |  4   | 3 2 1 0 |
>>>> + * |  Timestamp | ECN | SACK | WScale  |
>>>> + *
>>>> + * When we receive a valid cookie-ACK, we look at the echoed tsval (if
>>>> + * any) to figure out which TCP options we should use for the rebuilt
>>>> + * connection.
>>>> + *
>>>> + * A WScale setting of '0xf' (which is an invalid scaling value)
>>>> + * means that original syn did not include the TCP window scaling option.
>>>> + */
>>>> +#define TS_OPT_WSCALE_MASK	0xf
>>>> +#define TS_OPT_SACK		BIT(4)
>>>> +#define TS_OPT_ECN		BIT(5)
>>>> +/* There is no TS_OPT_TIMESTAMP:
>>>> + * if ACK contains timestamp option, we already know it was
>>>> + * requested/supported by the syn/synack exchange.
>>>> + */
>>>> +#define TSBITS	6
>>>> +#define TSMASK	(((__u32)1 << TSBITS) - 1)
>>>
>>> Personally I'd define all the values as hex constants instead of mixing
>>> and matching the defines.
>>>
>>> So probably just:
>>> #define TS_OPT_WSCALE_MASK	0x0f
>>> #define TS_OPT_SACK		0x10
>>> #define TS_OPT_ECN		0x20
>>> #define TSMASK                0x3f
>>
>> If you look at the above comment and then take a peek at the actual TS_OPT_*,
>> it is much easier to follow. Moreover, how is having TSMASK as 0x3f better?!
>> Currently, it is a constant calculated based upon TSBITS.
>
> TSMASK is also (TS_OPT_WSCALE_MASK | TS_OPT_SACK | TS_OPT_ECN) defining
> the values in hex makes this even more clear.

Right, that's your personal taste. ;) Besides, the definition of TSBITS/TSMASK
itself is not even altered here.

> Defining TSBITS from the mask would save the extra definition - which might
> be easier done by replacing the shifts with multiply/divide by (TSMASK + 1)
> (probably in a #define/inline function to make the code easier to read.

Sure, lets make it more complicated than it actually needs to be ... again,
I think the code is fine as is, sorry.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Nov. 3, 2014, 3:41 p.m. UTC | #5 
On Mon, 2014-11-03 at 14:01 +0100, Florian Westphal wrote:
> Was a bit more difficult to read than needed due to magic shifts;
> add defines and document the used encoding scheme.
> 
> Joint work with Daniel Borkmann.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>


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

Patch

diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 4ac7bca..c3792c0 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -19,10 +19,6 @@ 
 #include <net/tcp.h>
 #include <net/route.h>
 
-/* Timestamps: lowest bits store TCP options */
-#define TSBITS 6
-#define TSMASK (((__u32)1 << TSBITS) - 1)
-
 extern int sysctl_tcp_syncookies;
 
 static u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS] __read_mostly;
@@ -30,6 +26,30 @@  static u32 syncookie_secret[2][16-4+SHA_DIGEST_WORDS] __read_mostly;
 #define COOKIEBITS 24	/* Upper bits store count */
 #define COOKIEMASK (((__u32)1 << COOKIEBITS) - 1)
 
+/* TCP Timestamp: 6 lowest bits of timestamp sent in the cookie SYN-ACK
+ * stores TCP options:
+ *
+ * MSB                               LSB
+ * | 31 ...   6 |  5  |  4   | 3 2 1 0 |
+ * |  Timestamp | ECN | SACK | WScale  |
+ *
+ * When we receive a valid cookie-ACK, we look at the echoed tsval (if
+ * any) to figure out which TCP options we should use for the rebuilt
+ * connection.
+ *
+ * A WScale setting of '0xf' (which is an invalid scaling value)
+ * means that original syn did not include the TCP window scaling option.
+ */
+#define TS_OPT_WSCALE_MASK	0xf
+#define TS_OPT_SACK		BIT(4)
+#define TS_OPT_ECN		BIT(5)
+/* There is no TS_OPT_TIMESTAMP:
+ * if ACK contains timestamp option, we already know it was
+ * requested/supported by the syn/synack exchange.
+ */
+#define TSBITS	6
+#define TSMASK	(((__u32)1 << TSBITS) - 1)
+
 static DEFINE_PER_CPU(__u32 [16 + 5 + SHA_WORKSPACE_WORDS],
 		      ipv4_cookie_scratch);
 
@@ -67,9 +87,11 @@  __u32 cookie_init_timestamp(struct request_sock *req)
 
 	ireq = inet_rsk(req);
 
-	options = ireq->wscale_ok ? ireq->snd_wscale : 0xf;
-	options |= ireq->sack_ok << 4;
-	options |= ireq->ecn_ok << 5;
+	options = ireq->wscale_ok ? ireq->snd_wscale : TS_OPT_WSCALE_MASK;
+	if (ireq->sack_ok)
+		options |= TS_OPT_SACK;
+	if (ireq->ecn_ok)
+		options |= TS_OPT_ECN;
 
 	ts = ts_now & ~TSMASK;
 	ts |= options;
@@ -219,16 +241,13 @@  static inline struct sock *get_cookie_sock(struct sock *sk, struct sk_buff *skb,
  * additional tcp options in the timestamp.
  * This extracts these options from the timestamp echo.
  *
- * The lowest 4 bits store snd_wscale.
- * next 2 bits indicate SACK and ECN support.
- *
  * return false if we decode an option that should not be.
  */
 bool cookie_check_timestamp(struct tcp_options_received *tcp_opt,
 			struct net *net, bool *ecn_ok)
 {
 	/* echoed timestamp, lowest bits contain options */
-	u32 options = tcp_opt->rcv_tsecr & TSMASK;
+	u32 options = tcp_opt->rcv_tsecr;
 
 	if (!tcp_opt->saw_tstamp)  {
 		tcp_clear_options(tcp_opt);
@@ -238,19 +257,20 @@  bool cookie_check_timestamp(struct tcp_options_received *tcp_opt,
 	if (!sysctl_tcp_timestamps)
 		return false;
 
-	tcp_opt->sack_ok = (options & (1 << 4)) ? TCP_SACK_SEEN : 0;
-	*ecn_ok = (options >> 5) & 1;
+	tcp_opt->sack_ok = (options & TS_OPT_SACK) ? TCP_SACK_SEEN : 0;
+	*ecn_ok = options & TS_OPT_ECN;
 	if (*ecn_ok && !net->ipv4.sysctl_tcp_ecn)
 		return false;
 
 	if (tcp_opt->sack_ok && !sysctl_tcp_sack)
 		return false;
 
-	if ((options & 0xf) == 0xf)
+	if ((options & TS_OPT_WSCALE_MASK) == TS_OPT_WSCALE_MASK)
 		return true; /* no window scaling */
 
 	tcp_opt->wscale_ok = 1;
-	tcp_opt->snd_wscale = options & 0xf;
+	tcp_opt->snd_wscale = options & TS_OPT_WSCALE_MASK;
+
 	return sysctl_tcp_window_scaling != 0;
 }
 EXPORT_SYMBOL(cookie_check_timestamp);