netdev - Re: [PATCH net-next] selftests: drv-net: test drivers sleeping in ndo_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <677aab862c9b3_15102629427@willemb.c.googlers.com.notmuch>
Date: Sun, 05 Jan 2025 10:55:50 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, 
 davem@...emloft.net
Cc: netdev@...r.kernel.org, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 Jakub Kicinski <kuba@...nel.org>, 
 shuah@...nel.org, 
 willemb@...gle.com, 
 petrm@...dia.com, 
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next] selftests: drv-net: test drivers sleeping in
 ndo_get_stats64

Jakub Kicinski wrote:
> Most of our tests use rtnetlink to read device stats, so they
> don't expose the drivers much to paths in which device stats
> are read under RCU. Add tests which hammer profcs reads to
> make sure drivers:
>  - don't sleep while reporting stats,
>  - can handle parallel reads,
>  - can handle device going down while reading.
> 
> Set ifname on the env class in NetDrvEnv, we already do that
> in NetDrvEpEnv.
> 
>   KTAP version 1
>   1..7
>   ok 1 stats.check_pause
>   ok 2 stats.check_fec
>   ok 3 stats.pkt_byte_sum
>   ok 4 stats.qstat_by_ifindex
>   ok 5 stats.check_down
>   ok 6 stats.procfs_hammer
>   # completed up/down cycles: 6
>   ok 7 stats.procfs_downup_hammer
>   # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Reviewed-by: Willem de Bruijn <willemb@...gle.com>

Two tiny comments, neither cause for respin.

> +@...t_disruptive
> +def procfs_downup_hammer(cfg) -> None:
> +    """
> +    Reading stats via procfs only holds the RCU lock, drivers often try
> +    to sleep when reading the stats, or don't protect against races.
> +    """
> +    # Max out the queues, we'll flip between max an 1

s/an/and/

> +    channels = ethnl.channels_get({'header': {'dev-index': cfg.ifindex}})
> +    if channels['combined-count'] == 0:
> +        rx_type = 'rx'
> +    else:
> +        rx_type = 'combined'
> +    cur_queue_cnt = channels[f'{rx_type}-count']
> +    max_queue_cnt = channels[f'{rx_type}-max']
> +
> +    cmd(f"ethtool -L {cfg.ifname} {rx_type} {max_queue_cnt}")
> +    defer(cmd, f"ethtool -L {cfg.ifname} {rx_type} {cur_queue_cnt}")
> +
> +    # Real test stats
> +    stats = __run_inf_loop("cat /proc/net/dev")
> +    defer(stats.kill)
> +
> +    ipset = f"ip link set dev {cfg.ifname}"
> +    defer(ip, f"link set dev {cfg.ifname} up")

unimportant: could reference ipset here too, as in below.

> +    # The "echo -n 1" lets us count iterations below
> +    updown = f"{ipset} down; sleep 0.05; {ipset} up; sleep 0.05; " + \
> +             f"ethtool -L {cfg.ifname} {rx_type} 1; " + \
> +             f"ethtool -L {cfg.ifname} {rx_type} {max_queue_cnt}; " + \
> +              "echo -n 1"