[PATCH 0/2] Fix bug #80 again

Stefano Brivio

21 Jun 2024 21 Jun '24

5:53 p.m.

Oops. Stefano Brivio (2): Revert "udp: Make rport calculation more local" udp: Reduce scope of rport in udp_invert_portmap() udp.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) -- 2.43.0

Show replies by date

Stefano Brivio

21 Jun 21 Jun

5:53 p.m.

New subject: [PATCH 1/2] Revert "udp: Make rport calculation more local"

This reverts commit c80fa6a6bb4415ad48f9e11424310875d0d99bc7, as it reintroduces the issue fixed by commit 1e6f92b995a9 ("udp: Fix 16-bit overflow in udp_invert_portmap()"). Reported-by: Laurent Jacquot Link: https://bugs.passt.top/show_bug.cgi?id=80 Signed-off-by: Stefano Brivio --- udp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/udp.c b/udp.c index e79ca93..8181900 100644 --- a/udp.c +++ b/udp.c @@ -279,9 +279,10 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd) "Forward and reverse delta arrays must have same size"); for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) { in_port_t delta = fwd->f.delta[i]; + in_port_t rport = i + delta; if (delta) - fwd->rdelta[i + delta] = NUM_PORTS - delta; + fwd->rdelta[rport] = NUM_PORTS - delta; } } -- 2.43.0

David Gibson

24 Jun 24 Jun

3:20 a.m.

New subject: [PATCH 1/2] Revert "udp: Make rport calculation more local"

On Fri, Jun 21, 2024 at 05:53:22PM +0200, Stefano Brivio wrote:

...

This reverts commit c80fa6a6bb4415ad48f9e11424310875d0d99bc7, as it reintroduces the issue fixed by commit 1e6f92b995a9 ("udp: Fix 16-bit overflow in udp_invert_portmap()").

Reported-by: Laurent Jacquot Link: https://bugs.passt.top/show_bug.cgi?id=80 Signed-off-by: Stefano Brivio

Argh, dammit. So, Reviewed-by: David Gibson .. but I think we really need to put a comment on this, otherwise one of us is likely to make the same mistake again, because that particular promotion rule is truly arcane.

...

--- udp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/udp.c b/udp.c index e79ca93..8181900 100644 --- a/udp.c +++ b/udp.c @@ -279,9 +279,10 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd) "Forward and reverse delta arrays must have same size"); for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) { in_port_t delta = fwd->f.delta[i]; + in_port_t rport = i + delta;

if (delta) - fwd->rdelta[i + delta] = NUM_PORTS - delta; + fwd->rdelta[rport] = NUM_PORTS - delta; } }

-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson

Stefano Brivio

9:08 p.m.

New subject: [PATCH 1/2] Revert "udp: Make rport calculation more local"

On Mon, 24 Jun 2024 11:20:48 +1000 David Gibson wrote:

...

On Fri, Jun 21, 2024 at 05:53:22PM +0200, Stefano Brivio wrote:

...
This reverts commit c80fa6a6bb4415ad48f9e11424310875d0d99bc7, as it reintroduces the issue fixed by commit 1e6f92b995a9 ("udp: Fix 16-bit overflow in udp_invert_portmap()").

Reported-by: Laurent Jacquot Link: https://bugs.passt.top/show_bug.cgi?id=80 Signed-off-by: Stefano Brivio

Argh, dammit. So,

Reviewed-by: David Gibson

.. but I think we really need to put a comment on this, otherwise one of us is likely to make the same mistake again, because that particular promotion rule is truly arcane.

Pushed with a comment (part of commit for 2/2), I hope it's clear enough now. -- Stefano

Stefano Brivio

21 Jun 21 Jun

5:53 p.m.

New subject: [PATCH 2/2] udp: Reduce scope of rport in udp_invert_portmap()

cppcheck 2.14 warns that the scope of the rport variable could be reduced: do that, as reverted commit c80fa6a6bb44 ("udp: Make rport calculation more local") did, but keep the temporary variable of in_port_t type, otherwise the sum gets promoted to int. Reported-by: David Gibson Signed-off-by: Stefano Brivio --- udp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/udp.c b/udp.c index 8181900..a854323 100644 --- a/udp.c +++ b/udp.c @@ -279,10 +279,12 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd) "Forward and reverse delta arrays must have same size"); for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) { in_port_t delta = fwd->f.delta[i]; - in_port_t rport = i + delta; - if (delta) + if (delta) { + in_port_t rport = i + delta; + fwd->rdelta[rport] = NUM_PORTS - delta; + } } } -- 2.43.0

David Gibson

24 Jun 24 Jun

3:21 a.m.

New subject: [PATCH 2/2] udp: Reduce scope of rport in udp_invert_portmap()

On Fri, Jun 21, 2024 at 05:53:23PM +0200, Stefano Brivio wrote:

...

cppcheck 2.14 warns that the scope of the rport variable could be reduced: do that, as reverted commit c80fa6a6bb44 ("udp: Make rport calculation more local") did, but keep the temporary variable of in_port_t type, otherwise the sum gets promoted to int.

Reported-by: David Gibson Signed-off-by: Stefano Brivio

Reviewed-by: David Gibson

...

--- udp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/udp.c b/udp.c index 8181900..a854323 100644 --- a/udp.c +++ b/udp.c @@ -279,10 +279,12 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd) "Forward and reverse delta arrays must have same size"); for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) { in_port_t delta = fwd->f.delta[i]; - in_port_t rport = i + delta;

- if (delta) + if (delta) { + in_port_t rport = i + delta; + fwd->rdelta[rport] = NUM_PORTS - delta; + } } }

-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson

326

Age (days ago)

329

Last active (days ago)

List overview

Download

5 comments

2 participants

participants (2)

David Gibson
Stefano Brivio