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=gOE9bMP+; 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 A1B105A026F for ; Fri, 31 Oct 2025 09:21:27 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1761898886; 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=lrf+S0wGux+NviTxvt+IBs37mcmRESO+KoTNMomssqA=; b=gOE9bMP+UgDZp3+3uIHrk1OA17g+kncZ+jOrXm2PNc6TLH+fJO6EaRSX61qivWxdw80nY0 k6uUVTliv5Pyjv/pboGzebdqnTc1ApGFG4kEeG7Rly4oRRObPS+NajOWz9qzwuqUDIDW24 MCUUdOxtFDEF3gf40xn3C03JgbZZDus= 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-373-x0cfq6g_Nv6M3hk85sqECg-1; Fri, 31 Oct 2025 04:21:24 -0400 X-MC-Unique: x0cfq6g_Nv6M3hk85sqECg-1 X-Mimecast-MFC-AGG-ID: x0cfq6g_Nv6M3hk85sqECg_1761898883 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-471001b980eso12624935e9.1 for ; Fri, 31 Oct 2025 01:21:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1761898883; x=1762503683; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=lrf+S0wGux+NviTxvt+IBs37mcmRESO+KoTNMomssqA=; b=kzefEB55nBHkNylH9j8g1kqyfW148BAj+r4+5C9++lHMQzlafZIzovarlO8AaLSdsT Tcv7AVDOpXBP732DHIKVeRqPtO673gB+sOTRrRGPzsFMxA4tWqGMChJper97RRUQO1Cv NhbGGpwVD/9MBtddlBmIMHaNA9ypdYOq1/8ScpH+wYRfbJyXNpgGBEya+/r0b8K2TAmK ZFDiA16QAmbmKiZ/W9+oC95zZ9gbFSsCwm8KyRLySgyG/hNc7I+7rXJH1f/pk76ICqxx eRVztuP30laXApdQMWwczrGgjZU6YYLyHnyXWiqABt1/brlWRuuSlhGaQ6quT50szj9T 2TnA== X-Gm-Message-State: AOJu0YxfYmdyokw7X3pkfT3M0tflcoXC4VPF5qsiRanAxF+iPSdf+uuH IGm9qwpKndKJhCa9k0uM449WgfhOR1QmoFCAAyOnRj/TzXPUMudKmEL2nSluFLoqgpga3zBJFCP bxB6U092BIqrztvmQT3YwLvcKs9VGvMu+wq+WkcV9l/wLTLHpGs96tw== X-Gm-Gg: ASbGncsSX83NxreXjsXNw1N7ctX0htPKjq7IpuJoK/HC/MtrSn9DBn/gzJcphUbtK4H 5SC5bhajgc1DUmszAqMNrsiMJajGOOongeDnDWidCMH1NVRk4lx2kccstImaVKtvdItuztBu6Tx RM82l77R8amkTxIkm1Did+sKmOYiX2CA4nbptyJFd0nagB3KCYU9+pF9+0G/5+WZkwavypDjjhx ELlCr2Xcn9h+JN0/GjZNdyhs0yxrTJUy7vrAHHvzxvE88TjobHmtWmfz0/OUqmVwyZkyUa3pA0t aWGydRY2Y+bNtKxlt1vriVWkqCVtIOyk3QBPaMapXR7EqMIXbYi6nQsy0LdqPuJuW5Buvh0bbx8 QsDe6+b46uw== X-Received: by 2002:a05:600d:835a:b0:46e:3709:d88a with SMTP id 5b1f17b1804b1-47730890ea5mr13528485e9.33.1761898883362; Fri, 31 Oct 2025 01:21:23 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHwmFKYoP0MIS3DB/uF3+H/7DoYMi/6jWngTggmUwVgmi7YNNHPfUxcBaKAVVHwV3/XPJsNPg== X-Received: by 2002:a05:600d:835a:b0:46e:3709:d88a with SMTP id 5b1f17b1804b1-47730890ea5mr13528275e9.33.1761898882697; Fri, 31 Oct 2025 01:21:22 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-47732fe1075sm19573425e9.11.2025.10.31.01.21.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 31 Oct 2025 01:21:22 -0700 (PDT) Date: Fri, 31 Oct 2025 09:21:20 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 4/8] fwd: Move port exclusion handling from procfs_scan_listen() to callers Message-ID: <20251031092120.07ad0392@elisabeth> In-Reply-To: References: <20251011044827.862757-1-david@gibson.dropbear.id.au> <20251011044827.862757-5-david@gibson.dropbear.id.au> <20251030212404.3eda9c82@elisabeth> 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: j7SgwPgahCgZsDsp-glDlzyBEqmkS6vO_S_8d0cXSTQ_1761898883 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: YQBF3Q6GCOR7TZU3LW4O6QEF6E4H4O22 X-Message-ID-Hash: YQBF3Q6GCOR7TZU3LW4O6QEF6E4H4O22 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 Fri, 31 Oct 2025 13:47:10 +1100 David Gibson wrote: > On Thu, Oct 30, 2025 at 09:24:04PM +0100, Stefano Brivio wrote: > > On Sat, 11 Oct 2025 15:48:23 +1100 > > David Gibson wrote: > > > > > To avoid forwarding loops, we need to exclude certain ports from being > > > auto-forwarded. To accomplish this, procfs_scan_listen() takes a bitmap > > > of exclusions. As it detects each port, it checks against that bitmap. > > > This is a complicated way of accomplishing what we need. We can instead > > > mask out the excluded ports in the callers using a new bitmap_andc() > > > helper. > > > > > > Signed-off-by: David Gibson > > > --- > > > fwd.c | 32 ++++++++++++++------------------ > > > util.c | 23 +++++++++++++++++++++++ > > > util.h | 1 + > > > 3 files changed, 38 insertions(+), 18 deletions(-) > > > > > > diff --git a/fwd.c b/fwd.c > > > index 19309f14..c7b768d5 100644 > > > --- a/fwd.c > > > +++ b/fwd.c > > > @@ -110,13 +110,11 @@ bool fwd_port_is_ephemeral(in_port_t port) > > > * @fd: fd for relevant /proc/net file > > > * @lstate: Code for listening state to scan for > > > * @map: Bitmap where numbers of ports in listening state will be set > > > - * @exclude: Bitmap of ports to exclude from setting (and clear) > > > * > > > * #syscalls:pasta lseek > > > * #syscalls:pasta ppc64le:_llseek ppc64:_llseek arm:_llseek > > > */ > > > -static void procfs_scan_listen(int fd, unsigned int lstate, > > > - uint8_t *map, const uint8_t *exclude) > > > +static void procfs_scan_listen(int fd, unsigned int lstate, uint8_t *map) > > > { > > > struct lineread lr; > > > unsigned long port; > > > @@ -141,10 +139,7 @@ static void procfs_scan_listen(int fd, unsigned int lstate, > > > if (state != lstate) > > > continue; > > > > > > - if (bitmap_isset(exclude, port)) > > > - bitmap_clear(map, port); > > > - else > > > - bitmap_set(map, port); > > > + bitmap_set(map, port); > > > } > > > } > > > > > > @@ -157,8 +152,9 @@ static void fwd_scan_ports_tcp(struct fwd_ports *fwd, > > > const struct fwd_ports *rev) > > > { > > > memset(fwd->map, 0, PORT_BITMAP_SIZE); > > > - procfs_scan_listen(fwd->scan4, TCP_LISTEN, fwd->map, rev->map); > > > - procfs_scan_listen(fwd->scan6, TCP_LISTEN, fwd->map, rev->map); > > > + procfs_scan_listen(fwd->scan4, TCP_LISTEN, fwd->map); > > > + procfs_scan_listen(fwd->scan6, TCP_LISTEN, fwd->map); > > > + bitmap_andc(fwd->map, PORT_BITMAP_SIZE, fwd->map, rev->map); > > > > I'm not entirely sure why I implemented it this way in 9657b6ed05cc > > ("conf, tcp: Periodic detection of bound ports for pasta port > > forwarding"). > > > > I guess the original idea was that only procfs_scan_listen() would > > manipulate bitmaps, for whatever reason. In any case, I'm fairly > > sure that this is equivalent (it obviously look like it, but I'm > > having a hard time to convince myself because of the weird way I > > implemented it originally). > > > > > } > > > > > > /** > > > @@ -173,26 +169,26 @@ static void fwd_scan_ports_udp(struct fwd_ports *fwd, > > > const struct fwd_ports *tcp_fwd, > > > const struct fwd_ports *tcp_rev) > > > { > > > - uint8_t exclude[PORT_BITMAP_SIZE]; > > > - > > > - bitmap_or(exclude, PORT_BITMAP_SIZE, rev->map, tcp_rev->map); > > > - > > > memset(fwd->map, 0, PORT_BITMAP_SIZE); > > > - procfs_scan_listen(fwd->scan4, UDP_LISTEN, fwd->map, exclude); > > > - procfs_scan_listen(fwd->scan6, UDP_LISTEN, fwd->map, exclude); > > > + procfs_scan_listen(fwd->scan4, UDP_LISTEN, fwd->map); > > > + procfs_scan_listen(fwd->scan6, UDP_LISTEN, fwd->map); > > > > > > /* Also forward UDP ports with the same numbers as bound TCP ports. > > > * This is useful for a handful of protocols (e.g. iperf3) where a TCP > > > * control port is used to set up transfers on a corresponding UDP > > > * port. > > > - * > > > + */ > > > + procfs_scan_listen(tcp_fwd->scan4, TCP_LISTEN, fwd->map); > > > + procfs_scan_listen(tcp_fwd->scan6, TCP_LISTEN, fwd->map); > > > + > > > + /* > > > * This means we need to skip numbers of TCP ports bound on the other > > > > Nit: /* This ... > > Fixed. > > > > * side, too. Otherwise, we would detect corresponding UDP ports as > > > * bound and try to forward them from the opposite side, but it's > > > * already us handling them. > > > */ > > > - procfs_scan_listen(tcp_fwd->scan4, TCP_LISTEN, fwd->map, exclude); > > > - procfs_scan_listen(tcp_fwd->scan6, TCP_LISTEN, fwd->map, exclude); > > > + bitmap_andc(fwd->map, PORT_BITMAP_SIZE, fwd->map, rev->map); > > > + bitmap_andc(fwd->map, PORT_BITMAP_SIZE, fwd->map, tcp_rev->map); > > > } > > > > > > /** > > > diff --git a/util.c b/util.c > > > index c492f904..eabadf7d 100644 > > > --- a/util.c > > > +++ b/util.c > > > @@ -322,6 +322,7 @@ void bitmap_set(uint8_t *map, unsigned bit) > > > * @map: Pointer to bitmap > > > * @bit: Bit number to clear > > > */ > > > +/* cppcheck-suppress unusedFunction */ > > > void bitmap_clear(uint8_t *map, unsigned bit) > > > { > > > unsigned long *word = (unsigned long *)map + BITMAP_WORD(bit); > > > @@ -351,6 +352,7 @@ bool bitmap_isset(const uint8_t *map, unsigned bit) > > > * @a: First operand > > > * @b: Second operand > > > */ > > > +/* cppcheck-suppress unusedFunction */ > > > > Should we just drop those? git stores them for us. > > Eh, maybe? It felt weird to remove a function that seemed like it > belonged in the bitmap "library" of functions. Plus at the time I > thought I might use it again later in the series. I didn't but I > still might use it relatively soon in follow up series. Sure. > > > void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint8_t *b) > > > { > > > unsigned long *dw = (unsigned long *)dst; > > > @@ -365,6 +367,27 @@ void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint8_t *b) > > > dst[i] = a[i] | b[i]; > > > } > > > > > > +/** > > > + * bitmap_andc() - Logical conjunction with complement (AND NOT) of bitmap > > > > Nit: this function name mixes classic logic terminology (conjunction, > > complement) and operator names (and, not), which makes it hard to guess, > > I think. > > That's my POWER background showing: > https://www.ibm.com/docs/en/aix/7.3.0?topic=set-andc-complement-instruction I obviously don't have a POWER background but I do remember a couple of instructions simply because I found their names hilariously dissonant (other than finding them in whatever disassembly I was looking at). My favourite mouthful is RLWIMI: https://www.ibm.com/docs/en/openxl-c-and-cpp-aix/17.1.3?topic=rf-rldimi-builtin-ppc-rldimi-rlwimi-builtin-ppc-rlwimi but ANDC is also remarkable (cf. ANDN in the successful CISC set, which obviously sounds like the only right choice to me). -- Stefano