Re: [PATCH net] net: octeontx2-pf: Fix UB in shift operation

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-07-27 at 17:16:24, Sergey V. Frolov (Sergey.V.Frolov@kaspersky.com) wrote:
> On Mon, 2026-07-27 at 08:57 +0530, Ratheesh Kannoth wrote:
> >
> > >
> > > Signed-off-by: Sergey.V.Frolov <Sergey.V.Frolov@kaspersky.com>
> > > ---
> > >  drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c | 11
> > > ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > > index 40162b08014d..39bd73a214af 100644
> > > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > > @@ -53,10 +53,15 @@ static void otx2_get_egress_burst_cfg(struct
> > > otx2_nic *nic, u32 burst,
> > >       if (burst) {
> > >               *burst_exp = ilog2(burst) ? ilog2(burst) - 1 : 0;
> > >               tmp = burst - rounddown_pow_of_two(burst);
> > > -             if (burst < max_mantissa)
> > > +             if (burst <= max_mantissa) {
> >
> > Thanks.
> > can we handle 255 case with <= ? it looks simple to me.
>
> Thank you for reviewing the patch. I see that your simplification works
> with the current value of max_mantissa (which is 255).
>
> However, my concern is that this approach introduces an implicit
> dependency on the fact that max_mantissa never being changed. If in the
> future someone updates this constant (e.g. to support new hardware),
> the shift operation could become invalid again, and the UB would
> return. if max_mantissa becomes 127, then UB would reappear for burst
> values in the range 128-255 (where burst_exp would be 6, making
> *burst_exp - 7 = -1).
>
> My original patch explicitly handles both cases (*burst_exp >= 7 and <
> 7), so it's robust regardless of max_mantissa value. I'd prefer to keep
> the explicit check to avoid introducing a hidden dependency that might
> affect future maintainability.
>
> What do you think?

Thank you for the clarification.

I appreciate the robustness argument, but I'd like to provide some
hardware context that I think is relevant here.

On newer platforms (CN10K/CN20K), max_mantissa is 32767 — significantly
larger than the current OTX2/98xx value of 255. Given this trend, a
future reduction of max_mantissa below 256 would represent a major
architectural regression and is, in practice, highly unlikely.

It is also worth noting that in the proposed patch, the new else branch:

*burst_mantissa = tmp * (1ULL << (7 - *burst_exp));
is entirely unreachable dead code on CN10K/CN20K.

Rather than carrying dead code to guard against a hypothetical future
scenario, Could you consider following minimal approach:

1) Fix the off-by-one boundary condition by changing:
if (burst < max_mantissa)
to
if (burst <= max_mantissa)

2) Add a WARN_ON(*burst_exp < 7) before the else branch as an
explicit safeguard. This ensures that if max_mantissa ever changes
in a way that reintroduces this condition, it will be immediately
caught at runtime rather than silently triggering UB.

This keeps the fix minimal and correct for all current platforms, while
still providing a clear safety net against future regressions — without
adding unreachable code paths.