diff mbox series

[Openvpn-devel,S] Change in openvpn[master]: Minor fix to process_ip_header

Message ID a91d834e2d9f284b0278cf9940f50dd0a539f057-HTML@gerrit.openvpn.net
State Superseded
Headers
Series [Openvpn-devel,S] Change in openvpn[master]: Minor fix to process_ip_header |

Commit Message

plaisthos (Code Review) Feb. 15, 2024, 3:59 p.m. UTC 
  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

Comments

Gert Doering Feb. 15, 2024, 4:17 p.m. UTC | #1 
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
Gianmarco De Gregori Feb. 15, 2024, 7:50 p.m. UTC | #2 
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
Antonio Quartulli Feb. 16, 2024, 2 p.m. UTC | #3 
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,
Antonio Quartulli Feb. 19, 2024, 1:08 p.m. UTC | #4 
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,
Gert Doering Feb. 19, 2024, 1:12 p.m. UTC | #5 
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
Antonio Quartulli Feb. 19, 2024, 1:23 p.m. UTC | #6 
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,
Gert Doering Feb. 19, 2024, 1:49 p.m. UTC | #7 
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 mbox series

Patch

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);