| Message ID | 20221026123216.1575440-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipvs: use explicitly signed chars | expand |
Hello, On Wed, 26 Oct 2022, Jason A. Donenfeld wrote: > The `char` type with no explicit sign is sometimes signed and sometimes > unsigned. This code will break on platforms such as arm, where char is > unsigned. So mark it here as explicitly signed, so that the > todrop_counter decrement and subsequent comparison is correct. > > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > Cc: Julian Anastasov <ja@ssi.bg> > Cc: Simon Horman <horms@verge.net.au> > Cc: stable@vger.kernel.org > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> Looks good to me for -next, thanks! Acked-by: Julian Anastasov <ja@ssi.bg> > --- > net/netfilter/ipvs/ip_vs_conn.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c > index 8c04bb57dd6f..7c4866c04343 100644 > --- a/net/netfilter/ipvs/ip_vs_conn.c > +++ b/net/netfilter/ipvs/ip_vs_conn.c > @@ -1249,40 +1249,40 @@ static const struct seq_operations ip_vs_conn_sync_seq_ops = { > .next = ip_vs_conn_seq_next, > .stop = ip_vs_conn_seq_stop, > .show = ip_vs_conn_sync_seq_show, > }; > #endif > > > /* Randomly drop connection entries before running out of memory > * Can be used for DATA and CTL conns. For TPL conns there are exceptions: > * - traffic for services in OPS mode increases ct->in_pkts, so it is supported > * - traffic for services not in OPS mode does not increase ct->in_pkts in > * all cases, so it is not supported > */ > static inline int todrop_entry(struct ip_vs_conn *cp) > { > /* > * The drop rate array needs tuning for real environments. > * Called from timer bh only => no locking > */ > - static const char todrop_rate[9] = {0, 1, 2, 3, 4, 5, 6, 7, 8}; > - static char todrop_counter[9] = {0}; > + static const signed char todrop_rate[9] = {0, 1, 2, 3, 4, 5, 6, 7, 8}; > + static signed char todrop_counter[9] = {0}; > int i; > > /* if the conn entry hasn't lasted for 60 seconds, don't drop it. > This will leave enough time for normal connection to get > through. */ > if (time_before(cp->timeout + jiffies, cp->timer.expires + 60*HZ)) > return 0; > > /* Don't drop the entry if its number of incoming packets is not > located in [0, 8] */ > i = atomic_read(&cp->in_pkts); > if (i > 8 || i < 0) return 0; > > if (!todrop_rate[i]) return 0; > if (--todrop_counter[i] > 0) return 0; > > todrop_counter[i] = todrop_rate[i]; > return 1; > } > -- > 2.38.1 Regards -- Julian Anastasov <ja@ssi.bg>
On Wed, Oct 26, 2022 at 05:20:03PM +0300, Julian Anastasov wrote: > > Hello, > > On Wed, 26 Oct 2022, Jason A. Donenfeld wrote: > > > The `char` type with no explicit sign is sometimes signed and sometimes > > unsigned. This code will break on platforms such as arm, where char is > > unsigned. So mark it here as explicitly signed, so that the > > todrop_counter decrement and subsequent comparison is correct. > > > > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > > Cc: Julian Anastasov <ja@ssi.bg> > > Cc: Simon Horman <horms@verge.net.au> > > Cc: stable@vger.kernel.org > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > > Looks good to me for -next, thanks! This is actually net.git material, not net-next.git material, considering it fixes a bug on arm and many other archs, and is marked with a stable@ tag. > > Acked-by: Julian Anastasov <ja@ssi.bg> > > > --- > > net/netfilter/ipvs/ip_vs_conn.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c > > index 8c04bb57dd6f..7c4866c04343 100644 > > --- a/net/netfilter/ipvs/ip_vs_conn.c > > +++ b/net/netfilter/ipvs/ip_vs_conn.c > > @@ -1249,40 +1249,40 @@ static const struct seq_operations ip_vs_conn_sync_seq_ops = { > > .next = ip_vs_conn_seq_next, > > .stop = ip_vs_conn_seq_stop, > > .show = ip_vs_conn_sync_seq_show, > > }; > > #endif > > > > > > /* Randomly drop connection entries before running out of memory > > * Can be used for DATA and CTL conns. For TPL conns there are exceptions: > > * - traffic for services in OPS mode increases ct->in_pkts, so it is supported > > * - traffic for services not in OPS mode does not increase ct->in_pkts in > > * all cases, so it is not supported > > */ > > static inline int todrop_entry(struct ip_vs_conn *cp) > > { > > /* > > * The drop rate array needs tuning for real environments. > > * Called from timer bh only => no locking > > */ > > - static const char todrop_rate[9] = {0, 1, 2, 3, 4, 5, 6, 7, 8}; > > - static char todrop_counter[9] = {0}; > > + static const signed char todrop_rate[9] = {0, 1, 2, 3, 4, 5, 6, 7, 8}; > > + static signed char todrop_counter[9] = {0}; > > int i; > > > > /* if the conn entry hasn't lasted for 60 seconds, don't drop it. > > This will leave enough time for normal connection to get > > through. */ > > if (time_before(cp->timeout + jiffies, cp->timer.expires + 60*HZ)) > > return 0; > > > > /* Don't drop the entry if its number of incoming packets is not > > located in [0, 8] */ > > i = atomic_read(&cp->in_pkts); > > if (i > 8 || i < 0) return 0; > > > > if (!todrop_rate[i]) return 0; > > if (--todrop_counter[i] > 0) return 0; > > > > todrop_counter[i] = todrop_rate[i]; > > return 1; > > } > > -- > > 2.38.1 > > Regards > > -- > Julian Anastasov <ja@ssi.bg> >
Hello, On Wed, 26 Oct 2022, Jason A. Donenfeld wrote: > On Wed, Oct 26, 2022 at 05:20:03PM +0300, Julian Anastasov wrote: > > > > Hello, > > > > On Wed, 26 Oct 2022, Jason A. Donenfeld wrote: > > > > > The `char` type with no explicit sign is sometimes signed and sometimes > > > unsigned. This code will break on platforms such as arm, where char is > > > unsigned. So mark it here as explicitly signed, so that the > > > todrop_counter decrement and subsequent comparison is correct. > > > > > > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > > > Cc: Julian Anastasov <ja@ssi.bg> > > > Cc: Simon Horman <horms@verge.net.au> > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > > > > Looks good to me for -next, thanks! > > This is actually net.git material, not net-next.git material, > considering it fixes a bug on arm and many other archs, and is marked > with a stable@ tag. OK. As algorithm is not SMP safe, the problem is not just for the first 256 packets on these platforms. Regards -- Julian Anastasov <ja@ssi.bg>
Hi Pablo, On Wed, Oct 26, 2022 at 2:34 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > The `char` type with no explicit sign is sometimes signed and sometimes > unsigned. This code will break on platforms such as arm, where char is > unsigned. So mark it here as explicitly signed, so that the > todrop_counter decrement and subsequent comparison is correct. > > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > Cc: Julian Anastasov <ja@ssi.bg> > Cc: Simon Horman <horms@verge.net.au> > Cc: stable@vger.kernel.org > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> Wondering if you planned on taking this into nf.git for 6.1? Thanks, Jason
On Wed, Oct 26, 2022 at 02:32:16PM +0200, Jason A. Donenfeld wrote: > The `char` type with no explicit sign is sometimes signed and sometimes > unsigned. This code will break on platforms such as arm, where char is > unsigned. So mark it here as explicitly signed, so that the > todrop_counter decrement and subsequent comparison is correct. Applied, thanks
diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c index 8c04bb57dd6f..7c4866c04343 100644 --- a/net/netfilter/ipvs/ip_vs_conn.c +++ b/net/netfilter/ipvs/ip_vs_conn.c @@ -1249,40 +1249,40 @@ static const struct seq_operations ip_vs_conn_sync_seq_ops = { .next = ip_vs_conn_seq_next, .stop = ip_vs_conn_seq_stop, .show = ip_vs_conn_sync_seq_show, }; #endif /* Randomly drop connection entries before running out of memory * Can be used for DATA and CTL conns. For TPL conns there are exceptions: * - traffic for services in OPS mode increases ct->in_pkts, so it is supported * - traffic for services not in OPS mode does not increase ct->in_pkts in * all cases, so it is not supported */ static inline int todrop_entry(struct ip_vs_conn *cp) { /* * The drop rate array needs tuning for real environments. * Called from timer bh only => no locking */ - static const char todrop_rate[9] = {0, 1, 2, 3, 4, 5, 6, 7, 8}; - static char todrop_counter[9] = {0}; + static const signed char todrop_rate[9] = {0, 1, 2, 3, 4, 5, 6, 7, 8}; + static signed char todrop_counter[9] = {0}; int i; /* if the conn entry hasn't lasted for 60 seconds, don't drop it. This will leave enough time for normal connection to get through. */ if (time_before(cp->timeout + jiffies, cp->timer.expires + 60*HZ)) return 0; /* Don't drop the entry if its number of incoming packets is not located in [0, 8] */ i = atomic_read(&cp->in_pkts); if (i > 8 || i < 0) return 0; if (!todrop_rate[i]) return 0; if (--todrop_counter[i] > 0) return 0; todrop_counter[i] = todrop_rate[i]; return 1; }
The `char` type with no explicit sign is sometimes signed and sometimes unsigned. This code will break on platforms such as arm, where char is unsigned. So mark it here as explicitly signed, so that the todrop_counter decrement and subsequent comparison is correct. Cc: Pablo Neira Ayuso <pablo@netfilter.org> Cc: Julian Anastasov <ja@ssi.bg> Cc: Simon Horman <horms@verge.net.au> Cc: stable@vger.kernel.org Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- net/netfilter/ipvs/ip_vs_conn.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)