svn commit: r285782 - head/usr.bin/netstat

[Jason Unovitch]

On Jul 28, 2015 10:31 AM, "Gleb Smirnoff" <glebius at freebsd.org> wrote:
>
>   Mark, Jason,
>
> On Tue, Jul 21, 2015 at 11:57:39PM +0000, Mark Johnston wrote:
> M>   Fix counter reads on platforms where sizeof(uint64_t) !=
sizeof(uint64_t *).
> M>
> M>   In the kernel, structs such as tcpstat are manipulated as an array of
> M>   counter_u64_t (uint64_t *), but made visible to userland as an array
of
> M>   uint64_t. kread_counters() was previously copying the counter array
into
> M>   user space and sequentially overwriting each counter with its value.
This
> M>   mostly affects IPsec counters, as other counters are exported via
sysctl.
> M>
> M>   PR:                201700
> M>   Tested by: Jason Unovitch
>
> Thanks for fixing the bug after me.
>
> One question, though: why do you use sizeof(u_long) instead of size of a
> pointer?
>

Gleb,
Mark will have to provide more details on the choice of using
sizeof(u_long). I had tested the end result of the fix. All of our
discussion on the matter was in the PR.
https://bugs.freebsd.org/201700

It hasn't been MFC'd so I'd be more than happy to test the revision so we
can MFC both commits for the fix.

> M>@
==============================================================================
> M> --- head/usr.bin/netstat/main.c      Tue Jul 21 23:44:36 2015
(r285781)
> M> +++ head/usr.bin/netstat/main.c      Tue Jul 21 23:57:38 2015
(r285782)
> M> @@ -776,19 +776,31 @@ kread_counter(u_long addr)
> M>  int
> M>  kread_counters(u_long addr, void *buf, size_t size)
> M>  {
> M> -    uint64_t *c = buf;
> M> +    uint64_t *c;
> M> +    u_long *counters;
> M> +    size_t i, n;
> M>
> M>      if (kvmd_init() < 0)
> M>              return (-1);
> M>
> M> -    if (kread(addr, buf, size) < 0)
> M> +    if (size % sizeof(uint64_t) != 0) {
> M> +            xo_warnx("kread_counters: invalid counter set size");
> M>              return (-1);
> M> +    }
> M>
> M> -    while (size != 0) {
> M> -            *c = kvm_counter_u64_fetch(kvmd, *c);
> M> -            size -= sizeof(*c);
> M> -            c++;
> M> +    n = size / sizeof(uint64_t);
> M> +    if ((counters = malloc(n * sizeof(u_long))) == NULL)
> M> +            xo_err(-1, "malloc");
> M> +    if (kread(addr, counters, n * sizeof(u_long)) < 0) {
> M> +            free(counters);
> M> +            return (-1);
> M>      }
> M> +
> M> +    c = buf;
> M> +    for (i = 0; i < n; i++)
> M> +            c[i] = kvm_counter_u64_fetch(kvmd, counters[i]);
> M> +
> M> +    free(counters);
> M>      return (0);
> M>  }
>
> --
> Totus tuus, Glebius.

-Jason