| Message ID | a91d834e2d9f284b0278cf9940f50dd0a539f057-HTML@gerrit.openvpn.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,S] Change in openvpn[master]: Minor fix to process_ip_header | expand |
Hi, On Thu, Feb 15, 2024 at 03:59:02PM +0000, its_Giaan (Code Review) wrote: > if (buf->len > 0) > { > - /* > - * The --passtos and --mssfix options require > - * us to examine the IPv4 header. > - */ > - > - if (flags & (PIP_MSSFIX > -#if PASSTOS_CAPABILITY > - | PIPV4_PASSTOS > -#endif > - | PIPV4_CLIENT_NAT > - )) > + if (flags & PIP_OPT_MASK) NAK, as this is not the same thing. PIP_OPT_MASK will also match on the IPv6 flags, which are not something we need to test for here (= if only an IPv6 flag is active, why should we enter this branch?). gert
Hi, Il 15/02/2024 17:17, Gert Doering ha scritto: > Hi, > > On Thu, Feb 15, 2024 at 03:59:02PM +0000, its_Giaan (Code Review) wrote: >> if (buf->len > 0) >> { >> - /* >> - * The --passtos and --mssfix options require >> - * us to examine the IPv4 header. >> - */ >> - >> - if (flags & (PIP_MSSFIX >> -#if PASSTOS_CAPABILITY >> - | PIPV4_PASSTOS >> -#endif >> - | PIPV4_CLIENT_NAT >> - )) >> + if (flags & PIP_OPT_MASK) > NAK, as this is not the same thing. PIP_OPT_MASK will also match on > the IPv6 flags, which are not something we need to test for here (= if > only an IPv6 flag is active, why should we enter this branch?). I had the feeling that it was wrong in fact. Thanks for your feedback. Cheers
Hi, On 15/02/2024 17:17, Gert Doering wrote: > Hi, > > On Thu, Feb 15, 2024 at 03:59:02PM +0000, its_Giaan (Code Review) wrote: >> if (buf->len > 0) >> { >> - /* >> - * The --passtos and --mssfix options require >> - * us to examine the IPv4 header. >> - */ >> - >> - if (flags & (PIP_MSSFIX >> -#if PASSTOS_CAPABILITY >> - | PIPV4_PASSTOS >> -#endif >> - | PIPV4_CLIENT_NAT >> - )) >> + if (flags & PIP_OPT_MASK) > > NAK, as this is not the same thing. PIP_OPT_MASK will also match on > the IPv6 flags, which are not something we need to test for here (= if > only an IPv6 flag is active, why should we enter this branch?). We need to enter for either v4 or v6 flags, no? The check on whether the packet is v4 or v6 happens *inside* this if block. Am I wrong? Cheers,
Hi, On 16/02/2024 15:00, Antonio Quartulli wrote: > Hi, > > On 15/02/2024 17:17, Gert Doering wrote: >> Hi, >> >> On Thu, Feb 15, 2024 at 03:59:02PM +0000, its_Giaan (Code Review) wrote: >>> if (buf->len > 0) >>> { >>> - /* >>> - * The --passtos and --mssfix options require >>> - * us to examine the IPv4 header. >>> - */ >>> - >>> - if (flags & (PIP_MSSFIX >>> -#if PASSTOS_CAPABILITY >>> - | PIPV4_PASSTOS >>> -#endif >>> - | PIPV4_CLIENT_NAT >>> - )) >>> + if (flags & PIP_OPT_MASK) >> >> NAK, as this is not the same thing. PIP_OPT_MASK will also match on >> the IPv6 flags, which are not something we need to test for here (= if >> only an IPv6 flag is active, why should we enter this branch?). > > We need to enter for either v4 or v6 flags, no? > > The check on whether the packet is v4 or v6 happens *inside* this if > block. Am I wrong? I double checked the code once again and I think there is a bug in this check. Right now we check for some flags (say A and B) before entering the if-block. Once inside we take action on flag A, B, but also C, D and E. Now, if C, D and E are set, but A and B are not, we will never enter the if-block and execute the related code. Among this we have: * IPv6 flags (they are checked inside, but not on the outer guard) * PIPV4_EXTRACT_DHCP_ROUTER * PIPV6_IMCP_NOHOST_CLIENT * PIPV6_IMCP_NOHOST_SERVER (we also got an interesting typ0 in the last two). This said, I do believe this patch fixes these issues in one go as the new PIP_OPT_MASK will match all the flags mentioned above. It'd be interesting to test any behaviour expected to be triggered by PIPV4_EXTRACT_DHCP_ROUTER to confirm the theory. Cheers,
Hi, On Mon, Feb 19, 2024 at 02:08:56PM +0100, Antonio Quartulli wrote: > This said, I do believe this patch fixes these issues in one go as the new > PIP_OPT_MASK will match all the flags mentioned above. Yes, but then we do not need that if() anymore at all - if we go in there, no matter if we have anything to do (like, we have v6 bits set, and enter the v4 branch). Maybe that would be a more reasonable approach here... get rid of the umbrella if(), and check individual bits inside. It seems to be a micro-optimization "skip this branch if we have no single feature active", while at least MSSFIX is active by default. gert
Hi, On 19/02/2024 14:12, Gert Doering wrote: > Maybe that would be a more reasonable approach here... get rid of the > umbrella if(), and check individual bits inside. It seems to be a > micro-optimization "skip this branch if we have no single feature active", > while at least MSSFIX is active by default. Technically we still retain the "speed up" when all features are disabled (i.e. remove this and this from the config and gain some Mbps) But I do agree that the code complexity is not worth the gain, for a very narrow corner case. I vote for letting the umbrella if() go. Cheers,
Hi, On Mon, Feb 19, 2024 at 02:23:08PM +0100, Antonio Quartulli wrote: > On 19/02/2024 14:12, Gert Doering wrote: > > Maybe that would be a more reasonable approach here... get rid of the > > umbrella if(), and check individual bits inside. It seems to be a > > micro-optimization "skip this branch if we have no single feature active", > > while at least MSSFIX is active by default. > > Technically we still retain the "speed up" when all features are disabled > (i.e. remove this and this from the config and gain some Mbps) "some Mbps" - maybe not... "a few kbit/s" sounds more like it :-) gert
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 0443ca0..04bf407 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1649,17 +1649,7 @@ if (buf->len > 0) { - /* - * The --passtos and --mssfix options require - * us to examine the IPv4 header. - */ - - if (flags & (PIP_MSSFIX -#if PASSTOS_CAPABILITY - | PIPV4_PASSTOS -#endif - | PIPV4_CLIENT_NAT - )) + if (flags & PIP_OPT_MASK) { struct buffer ipbuf = *buf; if (is_ipv4(TUNNEL_TYPE(c->c1.tuntap), &ipbuf)) diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h index e19115e..8da1cc1 100644 --- a/src/openvpn/forward.h +++ b/src/openvpn/forward.h @@ -294,11 +294,13 @@ #define PIPV4_PASSTOS (1<<0) #define PIP_MSSFIX (1<<1) /* v4 and v6 */ -#define PIP_OUTGOING (1<<2) #define PIPV4_EXTRACT_DHCP_ROUTER (1<<3) #define PIPV4_CLIENT_NAT (1<<4) #define PIPV6_IMCP_NOHOST_CLIENT (1<<5) #define PIPV6_IMCP_NOHOST_SERVER (1<<6) +#define PIP_OPT_MASK 0xFFFF +#define PIP_OUTGOING (1<<16) + void process_ip_header(struct context *c, unsigned int flags, struct buffer *buf);
Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/525?usp=email to review the following change. Change subject: Minor fix to process_ip_header ...................................................................... Minor fix to process_ip_header Introduced a bitmask to replace the previous check for individual option bits with a check for all possible combinations of options. Fixes: Trac https://community.openvpn.net/openvpn/ticket/269 Change-Id: I4b5e8357d872c920efdb64632e9bce72cebee202 Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com> --- M src/openvpn/forward.c M src/openvpn/forward.h 2 files changed, 4 insertions(+), 12 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/25/525/1