From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=UplEFA2S; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id AF4EF5A004E for ; Tue, 13 Jan 2026 23:12:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1768342352; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=bwVrh48JWqKPSnhaG+9dZE/SNH/M56/+qAMFB5r4CZI=; b=UplEFA2SRdOO/z8tInTli4qd8sgvzECJKHQtDooomQSz+QliIytuPCrPyi1VjFEwOUQ9LH 4ga1u8+Hgry2fT7eSg54yrJxywbDVSa623+I5iLM8kssciyCZaiNpuf3zsf4WyN1iW3VR0 VFHBqXSA38Le5rR0rOEIuaTgVxkH1Iw= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-439-oiCVneKtPginTz9NQKuF5w-1; Tue, 13 Jan 2026 17:12:30 -0500 X-MC-Unique: oiCVneKtPginTz9NQKuF5w-1 X-Mimecast-MFC-AGG-ID: oiCVneKtPginTz9NQKuF5w_1768342349 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-47d5c7a2f54so1198085e9.1 for ; Tue, 13 Jan 2026 14:12:29 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768342349; x=1768947149; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=bwVrh48JWqKPSnhaG+9dZE/SNH/M56/+qAMFB5r4CZI=; b=sBbMnlzeCC9wGhclnrShCacB0ttSl3aI36e6wvxoTXuZ6/rZedv5jR4GBfRFy2jkKt fpJ2HF96fBYjTr3rkLJlbnuI9me/pVfNyoH+92r16sQyLoA1+1hMp3BnELtD/TM7y3hE +8CZGUINURGm7h3j7dY06k/YvsCeE3YfQ4gyJy3OhbOLquNQp7Pimx6UCgNRmbGrG0bY Cak6M24s1K2j9XsOuzsbNy2ZZl6awEYnzLLbV/Hp/ExrRRAXVKIwDuafQRdNy5H0+oa6 lehHPVFpOAIdonOl9CVEJBW6EqqxSSRrbXbA381XpvLn+1/XqoHhL53tXbHcBDAp9M1x g08w== X-Gm-Message-State: AOJu0YzrjbSTVYe9zbISUlXKyL1bb08StAdt1DHC80HbG0YPdmIuGgQP fPwWNqvoIDvnHb0W+O4pAFBy/qCvheisqzNsAWu92C18N7XyYhXuzOegwUmM7449uhn0J3+xiqw TZ+4XP9+wByPfn9xpRilb1b2GEKPJvQK7kcUqFLz74+n0KeC0H1GQXA== X-Gm-Gg: AY/fxX5FHTJ+dXifBy3o/hRlySujEI8K23AFZ4u3dG/lVYlyROaYH9cI2vCjkLyGDAt j7LqxBB7/zekL8P9Leci0Tds8D1x4ZsjlilQ4wPDnZHbwJ0j78LwH6aHpJECsQdPtmi1S8OWgsE agS8EA4p8U0g2Gd1swBdoRmba+r7AWw+Sjr0L88rvz0L9dJInRdBQpaatfjJuULhVwiHZw/uDc8 z9n5/yOQMhfQ0B99JcJbwLuYXaWpqN6OZY4E4Qi2X3c8T5nML7zUlfvQKDbOkLMgnRK6lB+NMf8 GcuHbiIcCKLVWa5Wsjew/I50HLd6VGuTBrYV7fBvqaEnHrLUdE+uzhf/EqBaDQbY4EGfkq+v1eS S/4ujkIOJFwcBPx2wlh1g X-Received: by 2002:a05:600c:3e86:b0:46f:a2ba:581f with SMTP id 5b1f17b1804b1-47ee37b5613mr5106085e9.16.1768342348517; Tue, 13 Jan 2026 14:12:28 -0800 (PST) X-Received: by 2002:a05:600c:3e86:b0:46f:a2ba:581f with SMTP id 5b1f17b1804b1-47ee37b5613mr5105915e9.16.1768342347985; Tue, 13 Jan 2026 14:12:27 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-47ee2a5e48asm5829245e9.20.2026.01.13.14.12.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 13 Jan 2026 14:12:27 -0800 (PST) Date: Tue, 13 Jan 2026 23:12:26 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 12/14] fwd: Remap ports based directly on forwarding rule Message-ID: <20260113231226.3b7fba24@elisabeth> In-Reply-To: <20260108022948.2657573-13-david@gibson.dropbear.id.au> References: <20260108022948.2657573-1-david@gibson.dropbear.id.au> <20260108022948.2657573-13-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: sh9esjohEcNGjRuCBA1boMlFiO0jgyBg9glyokiClos_1768342349 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: FRM3THO2F2YIY45WWXE3PR3NDDVFS2P3 X-Message-ID-Hash: FRM3THO2F2YIY45WWXE3PR3NDDVFS2P3 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 8 Jan 2026 13:29:46 +1100 David Gibson wrote: > Currently we remap port numbers based on the legacy delta[] array, which > is indexed only by original port number, not the listening address. Now > that we look up a forwarding rule entry in flow_target(), we can use this > entry to directly determine the correct remapped port. Implement this, > and remove the old delta[] array. > > Link: https://bugs.passt.top/show_bug.cgi?id=187 > > Signed-off-by: David Gibson > --- > flow.c | 7 ++++--- > fwd.c | 21 +++++++-------------- > fwd.h | 7 +++---- > 3 files changed, 14 insertions(+), 21 deletions(-) > > diff --git a/flow.c b/flow.c > index 045e9712..38f88732 100644 > --- a/flow.c > +++ b/flow.c > @@ -493,6 +493,7 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow, > struct flow_common *f = &flow->f; > const struct flowside *ini = &f->side[INISIDE]; > struct flowside *tgt = &f->side[TGTSIDE]; > + const struct fwd_rule *rule = NULL; Coverity Scan complains about two cases where this NULL rule would be passed to NAT functions dereferencing it. They should be both false positives because the rule is always set on pif_is_socket(...), but I wonder how fragile it is. Regardless, it would be nice to avoid adding further warnings, if it's cheap (ASSERT() or check on rule in fwd_nat_from*() functions?). Anyway, here you go: /home/sbrivio/passt/flow.c:535:3: Type: Explicit null dereferenced (FORWARD_NULL) /home/sbrivio/passt/flow.c:496:2: 1. assign_zero: Assigning: "rule" = "NULL". /home/sbrivio/passt/flow.c:499:2: 2. path: Condition "flow_new_entry == flow", taking true branch. /home/sbrivio/passt/flow.c:499:2: 3. path: Condition "f->state == FLOW_STATE_INI", taking true branch. /home/sbrivio/passt/flow.c:500:2: 4. path: Condition "f->type == FLOW_TYPE_NONE", taking true branch. /home/sbrivio/passt/flow.c:501:2: 5. path: Condition "f->pif[0] != PIF_NONE", taking true branch. /home/sbrivio/passt/flow.c:501:2: 6. path: Condition "f->pif[1] == PIF_NONE", taking true branch. /home/sbrivio/passt/flow.c:502:2: 7. path: Condition "flow->f.state == FLOW_STATE_INI", taking true branch. /home/sbrivio/passt/flow.c:504:2: 8. path: Condition "pif_is_socket(f->pif[0])", taking false branch. /home/sbrivio/passt/flow.c:528:2: 9. path: Switch case value "PIF_SPLICE". /home/sbrivio/passt/flow.c:535:3: 10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_splice", which dereferences it. /home/sbrivio/passt/fwd.c:991:2: 10.1. path: Condition "!inany_is_loopback(&ini->eaddr)", taking false branch. /home/sbrivio/passt/fwd.c:991:2: 10.2. path: Condition "!inany_is_loopback(&ini->oaddr)", taking false branch. /home/sbrivio/passt/fwd.c:1008:2: 10.3. path: Condition "proto == IPPROTO_UDP", taking true branch. /home/sbrivio/passt/fwd.c:1012:2: 10.4. dereference: Dereferencing pointer "rule". /home/sbrivio/passt/flow.c:539:3: Type: Explicit null dereferenced (FORWARD_NULL) /home/sbrivio/passt/flow.c:496:2: 1. assign_zero: Assigning: "rule" = "NULL". /home/sbrivio/passt/flow.c:499:2: 2. path: Condition "flow_new_entry == flow", taking true branch. /home/sbrivio/passt/flow.c:499:2: 3. path: Condition "f->state == FLOW_STATE_INI", taking true branch. /home/sbrivio/passt/flow.c:500:2: 4. path: Condition "f->type == FLOW_TYPE_NONE", taking true branch. /home/sbrivio/passt/flow.c:501:2: 5. path: Condition "f->pif[0] != PIF_NONE", taking true branch. /home/sbrivio/passt/flow.c:501:2: 6. path: Condition "f->pif[1] == PIF_NONE", taking true branch. /home/sbrivio/passt/flow.c:502:2: 7. path: Condition "flow->f.state == FLOW_STATE_INI", taking true branch. /home/sbrivio/passt/flow.c:504:2: 8. path: Condition "pif_is_socket(f->pif[0])", taking false branch. /home/sbrivio/passt/flow.c:528:2: 9. path: Switch case value "PIF_HOST". /home/sbrivio/passt/flow.c:539:3: 10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_host", which dereferences it. /home/sbrivio/passt/fwd.c:1069:2: 10.1. dereference: Dereferencing pointer "rule". > uint8_t tgtpif = PIF_NONE; > > ASSERT(flow_new_entry == flow && f->state == FLOW_STATE_INI); > @@ -514,7 +515,7 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow, > else > goto nofwd; > > - if (!fwd_rule_search(fwd, ini)) { > + if (!(rule = fwd_rule_search(fwd, ini))) { > /* This shouldn't happen, because if there's no rule for > * it we should have no listening socket that would let > * us get here > @@ -531,11 +532,11 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow, > break; > > case PIF_SPLICE: > - tgtpif = fwd_nat_from_splice(c, proto, ini, tgt); > + tgtpif = fwd_nat_from_splice(rule, proto, ini, tgt); > break; > > case PIF_HOST: > - tgtpif = fwd_nat_from_host(c, proto, ini, tgt); > + tgtpif = fwd_nat_from_host(c, rule, proto, ini, tgt); > fwd_neigh_mac_get(c, &tgt->oaddr, f->tap_omac); > break; > default: > diff --git a/fwd.c b/fwd.c > index 633ba5db..7c4575ff 100644 > --- a/fwd.c > +++ b/fwd.c > @@ -405,7 +405,6 @@ void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags, > /* Fill in the legacy forwarding data structures to match the table */ > if (!(new->flags & FWD_SCAN)) > bitmap_set(fwd->map, port); > - fwd->delta[port] = new->to - new->first; > } > } > > @@ -978,7 +977,7 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto, > > /** > * fwd_nat_from_splice() - Determine to forward a flow from the splice interface > - * @c: Execution context > + * @rule: Forwarding rule to apply > * @proto: Protocol (IP L4 protocol number) > * @ini: Flow address information of the initiating side > * @tgt: Flow address information on the target side (updated) > @@ -986,7 +985,7 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto, > * Return: pif of the target interface to forward the flow to, PIF_NONE if the > * flow cannot or should not be forwarded at all. > */ > -uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto, > +uint8_t fwd_nat_from_splice(const struct fwd_rule *rule, uint8_t proto, > const struct flowside *ini, struct flowside *tgt) > { > if (!inany_is_loopback(&ini->eaddr) || > @@ -1010,11 +1009,7 @@ uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto, > /* But for UDP preserve the source port */ > tgt->oport = ini->eport; > > - tgt->eport = ini->oport; > - if (proto == IPPROTO_TCP) > - tgt->eport += c->tcp.fwd_out.delta[tgt->eport]; > - else if (proto == IPPROTO_UDP) > - tgt->eport += c->udp.fwd_out.delta[tgt->eport]; > + tgt->eport = ini->oport - rule->first + rule->to; This made me notice that the current documentation to struct fwd_rule doesn't really make it clear that n : 1 port mapping rules are not a thing, so, maybe, back to 2/14, instead of: * @to: Port number to forward port @first to. we could have perhaps: * @to: Target port for @first, port n maps to @to + (n - @first) ? By the way, I find this notation a bit complicated to figure out, I think that: rule->to + (ini->oport - rule->first) (redundant parentheses included) is actually clearer. Or maybe even with a temporary 'delta' variable. In both cases: the subtraction now happens in in_port_t, so I guess we should explicitly cast rule->first to int to be strictly correct. > return PIF_HOST; > } > @@ -1058,6 +1053,7 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr, > /** > * fwd_nat_from_host() - Determine to forward a flow from the host interface > * @c: Execution context > + * @rule: Forwarding rule to apply > * @proto: Protocol (IP L4 protocol number) > * @ini: Flow address information of the initiating side > * @tgt: Flow address information on the target side (updated) > @@ -1065,15 +1061,12 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr, > * Return: pif of the target interface to forward the flow to, PIF_NONE if the > * flow cannot or should not be forwarded at all. > */ > -uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto, > +uint8_t fwd_nat_from_host(const struct ctx *c, > + const struct fwd_rule *rule, uint8_t proto, > const struct flowside *ini, struct flowside *tgt) > { > /* Common for spliced and non-spliced cases */ > - tgt->eport = ini->oport; > - if (proto == IPPROTO_TCP) > - tgt->eport += c->tcp.fwd_in.delta[tgt->eport]; > - else if (proto == IPPROTO_UDP) > - tgt->eport += c->udp.fwd_in.delta[tgt->eport]; > + tgt->eport = ini->oport - rule->first + rule->to; Same as above. > > if (!c->no_splice && inany_is_loopback(&ini->eaddr) && > (proto == IPPROTO_TCP || proto == IPPROTO_UDP)) { > diff --git a/fwd.h b/fwd.h > index a10bdbb4..cfe9ed46 100644 > --- a/fwd.h > +++ b/fwd.h > @@ -82,7 +82,6 @@ enum fwd_ports_mode { > * @count: Number of forwarding rules > * @rules: Array of forwarding rules > * @map: Bitmap describing which ports are forwarded > - * @delta: Offset between the original destination and mapped port number > * @listen_sock_count: Number of entries used in @listen_socks > * @listen_socks: Listening sockets for forwarding > */ > @@ -93,7 +92,6 @@ struct fwd_ports { > unsigned count; > struct fwd_rule rules[MAX_FWD_RULES]; > uint8_t map[PORT_BITMAP_SIZE]; > - in_port_t delta[NUM_PORTS]; > unsigned listen_sock_count; > int listen_socks[MAX_LISTEN_SOCKS]; > }; > @@ -117,9 +115,10 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr, > union inany_addr *translated); > uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto, > const struct flowside *ini, struct flowside *tgt); > -uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto, > +uint8_t fwd_nat_from_splice(const struct fwd_rule *rule, uint8_t proto, > const struct flowside *ini, struct flowside *tgt); > -uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto, > +uint8_t fwd_nat_from_host(const struct ctx *c, > + const struct fwd_rule *rule, uint8_t proto, > const struct flowside *ini, struct flowside *tgt); > void fwd_neigh_table_update(const struct ctx *c, const union inany_addr *addr, > const uint8_t *mac, bool permanent); -- Stefano