Re: [PATCH net-next v4 00/17] Some pktgen fixes/improvments

[Peter Seiderer <ps.report@gmx.net>]

Hello Simon,

On Thu, 6 Feb 2025 13:51:54 +0000, Simon Horman <horms@kernel.org> wrote:

> On Wed, Feb 05, 2025 at 02:11:36PM +0100, Peter Seiderer wrote:
> > hile taking a look at '[PATCH net] pktgen: Avoid out-of-range in
> > get_imix_entries' ([1]) and '[PATCH net v2] pktgen: Avoid out-of-bounds access
> > in get_imix_entries' ([2], [3]) and doing some tests and code review I
> > detected that the /proc/net/pktgen/... parsing logic does not honour the
> > user given buffer bounds (resulting in out-of-bounds access).
> >
> > This can be observed e.g. by the following simple test (sometimes the
> > old/'longer' previous value is re-read from the buffer):
> >
> >         $ echo add_device lo@0 > /proc/net/pktgen/kpktgend_0
> >
> >         $ echo "min_pkt_size 12345" > /proc/net/pktgen/lo\@0 && grep min_pkt_size /proc/net/pktgen/lo\@0
> > Params: count 1000  min_pkt_size: 12345  max_pkt_size: 0
> > Result: OK: min_pkt_size=12345
> >
> >         $ echo -n "min_pkt_size 123" > /proc/net/pktgen/lo\@0 && grep min_pkt_size /proc/net/pktgen/lo\@0
> > Params: count 1000  min_pkt_size: 12345  max_pkt_size: 0
> > Result: OK: min_pkt_size=12345
> >
> >         $ echo "min_pkt_size 123" > /proc/net/pktgen/lo\@0 && grep min_pkt_size /proc/net/pktgen/lo\@0
> > Params: count 1000  min_pkt_size: 123  max_pkt_size: 0
> > Result: OK: min_pkt_size=123
> >
> > So fix the out-of-bounds access (and some minor findings) and add a simple
> > proc_net_pktgen selftest...
> >
> > Regards,
> > Peter
> >
> > Changes v3 -> v4:
> >  - add rev-by Simon Horman
> >  - new patch 'net: pktgen: use defines for the various dec/hex number parsing
> >    digits lengths' (suggested by Simon Horman)
> >  - replace C99 comment (suggested by Paolo Abeni)
> >  - drop available characters check in strn_len() (suggested by Paolo Abeni)
> >  - factored out patch 'net: pktgen: align some variable declarations to the
> >    most common pattern' (suggested by Paolo Abeni)
> >  - factored out patch 'net: pktgen: remove extra tmp variable (re-use len
> >    instead)' (suggested by Paolo Abeni)
> >  - factored out patch 'net: pktgen: remove some superfluous variable
> >    initializing' (suggested by Paolo Abeni)
> >  - factored out patch 'net: pktgen: fix mpls maximum labels list parsing'
> >    (suggested by Paolo Abeni)
> >  - factored out 'net: pktgen: hex32_arg/num_arg error out in case no
> >    characters are available' (suggested by Paolo Abeni)
> >  - factored out 'net: pktgen: num_arg error out in case no valid character
> >    is parsed' (suggested by Paolo Abeni)
>
> Hi Peter,
>
> Thanks for splitting up the patchset some more, I for one find it much
> easier to review them in this form.

Definitely!

>
> That said, we are now over the preferred maximum of 15 patches in a series.
> Perhaps the maintainers are ok with that, but I'd like to suggest breaking
> the series in two: The first 7 patches seem to be somewhat stable, from a
> review perspective, and could be posted as "part i"; And then the remaining
> patches could be posted as "part ii" once "part i" has been accepted.

Yes, the patch set evolved a little bit over the 'just fix some little strange
behavior'..., splitting it up will work for me for sure..., will do on next
patch set iteration...

Regards,
Peter

>
> As for the selftests (the last patch of the series). A version,
> trimmed down as appropriate, could be included in "part i", with a
> follow-up in "part ii". Or the cover note for "part i" could state that the
> selftests have been deferred to "part ii".
>
> Perhaps the maintainers have other ideas, if so hopefully they will comment
> here.