Hello Paolo,
On Tue, 4 Feb 2025 11:40:21 +0100, Paolo Abeni <pab...@redhat.com> wrote:
> Hi,
>
> On 2/3/25 6:01 PM, Peter Seiderer wrote:
> > @@ -806,6 +812,9 @@ static long num_arg(const char __user *user_buffer,
> > unsigned long maxlen,
> > if ((c >= '0') && (c <= '9')) {
> > *num *= 10;
> > *num += c - '0';
> > + } else if (i == 0) {
> > + // no valid character parsed, error out
>
> Minor nit: please don't use C99 comments, even for single line one.
Fixed...
>
> > + return -EINVAL;
> > } else
> > break;
> > }
> > @@ -816,6 +825,9 @@ static int strn_len(const char __user * user_buffer,
> > unsigned int maxlen)
> > {
> > int i;
> >
> > + if (!maxlen)
> > + return -EINVAL;
>
> It looks like this check is not needed? strn_len() will return 0 and the
> caller will read 0 bytes from the user_buffer.
Checked all call sites, your are right, fixed in next patch iteration...
>
> > @@ -882,39 +897,45 @@ static ssize_t get_imix_entries(const char __user
> > *buffer,
> > pkt_dev->imix_entries[pkt_dev->n_imix_entries].weight = weight;
> >
> > i += len;
> > + pkt_dev->n_imix_entries++;
> > +
> > + if (i >= maxlen)
> > + break;
> > if (get_user(c, &buffer[i]))
> > return -EFAULT;
> > -
> > i++;
> > - pkt_dev->n_imix_entries++;
> > } while (c == ' ');
> >
> > return i;
> > }
> >
> > -static ssize_t get_labels(const char __user *buffer, struct pktgen_dev
> > *pkt_dev)
> > +static ssize_t get_labels(const char __user *buffer, int maxlen, struct
> > pktgen_dev *pkt_dev)
> > {
> > unsigned int n = 0;
> > char c;
> > - ssize_t i = 0;
> > - int len;
> > + int i = 0, max, len;
>
> Minor nit: since you are touching the variables declaration, please fix
> them to respect the reverse christmas tree order.
Fixed...
>
> This patch is quite large and mixes several things. I'll split out at
> least the strn_len() caller fixes (possibly even the num_arg() and
> hex32_arg() ones) and the refactoring in pktgen_if_write().
Done in the next patch iteration....
Thanks for review!
Regards,
Peter
>
> /P
>
