linux-kernel - Re: [PATCH 6/6] net: ravb: Keep reverse order of operations in ravb

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: <716b433e-5e0c-4353-ea39-12cb4f3d50c4@omp.ru>
Date:   Mon, 27 Nov 2023 20:16:03 +0300
From:   Sergey Shtylyov <s.shtylyov@....ru>
To:     Claudiu <claudiu.beznea@...on.dev>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <richardcochran@...il.com>, <p.zabel@...gutronix.de>,
        <yoshihiro.shimoda.uh@...esas.com>, <geert+renesas@...der.be>,
        <wsa+renesas@...g-engineering.com>, <robh@...nel.org>,
        <biju.das.jz@...renesas.com>,
        <prabhakar.mahadev-lad.rj@...renesas.com>,
        <mitsuhiro.kimura.kc@...esas.com>, <masaru.nagai.vx@...esas.com>
CC:     <netdev@...r.kernel.org>, <linux-renesas-soc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 6/6] net: ravb: Keep reverse order of operations in
 ravb_remove()

On 11/27/23 12:04 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> 
> On RZ/G3S SMARC Carrier II board having RGMII connections b/w Ethernet
> MACs and PHYs it has been discovered that doing unbind/bind for ravb
> driver in a loop leads to wrong speed and duplex for Ethernet links and
> broken connectivity (the connectivity cannot be restored even with
> bringing interface down/up). Before doing unbind/bind the Ethernet
> interfaces were configured though systemd. The sh instructions used to
> do unbind/bind were:
> 
> $ cd /sys/bus/platform/drivers/ravb/
> $ while :; do echo 11c30000.ethernet > unbind ; \
>   echo 11c30000.ethernet > bind; done
> 
> It has been discovered that there is a race b/w IOCTLs initialized by
> systemd at the response of success binding and the
> "ravb_write(ndev, CCC_OPC_RESET, CCC)" instruction in ravb_remove() as

   s/instruction/call/, perhaps?

> follows:
> 
> 1/ as a result of bind success the user space open/configures the
>    interfaces tough an IOCTL; the following stack trace has been
>    identified on RZ/G3S:
> 
> Call trace:
> dump_backtrace+0x9c/0x100
> show_stack+0x20/0x38
> dump_stack_lvl+0x48/0x60
> dump_stack+0x18/0x28
> ravb_open+0x70/0xa58
> __dev_open+0xf4/0x1e8
> __dev_change_flags+0x198/0x218
> dev_change_flags+0x2c/0x80
> devinet_ioctl+0x640/0x708
> inet_ioctl+0x1e4/0x200
> sock_do_ioctl+0x50/0x108
> sock_ioctl+0x240/0x358
> __arm64_sys_ioctl+0xb0/0x100
> invoke_syscall+0x50/0x128
> el0_svc_common.constprop.0+0xc8/0xf0
> do_el0_svc+0x24/0x38
> el0_svc+0x34/0xb8
> el0t_64_sync_handler+0xc0/0xc8
> el0t_64_sync+0x190/0x198
> 
> 2/ this call may execute concurrently with ravb_remove() as the
>    unbind/bind operation was executed in a loop
> 3/ if the operation mode is changed to RESET (though

   Through?

>    ravb_write(ndev, CCC_OPC_RESET, CCC) instruction in ravb_remove())

   s/instruction/call/, perhaps?

>    while the above ravb_open() is in progress it may lead to MAC
>    (or PHY, or MAC-PHY connection, the right point hasn't been identified
>    at the moment) to be broken, thus the Ethernet connectivity fails to
>    restore.
> 
> The simple fix for this is to move ravb_write(ndev, CCC_OPC_RESET, CCC))
> after unregister_netdev() to avoid resetting the controller while the
> netdev interface is still registered.
> 
> To avoid future issues in ravb_remove(), the patch follows the proper order
> of operations in ravb_remove(): reverse order compared with ravb_probe().
> This avoids described races as the IOCTLs as well as unregister_netdev()
> (called now at the beginning of ravb_remove()) calls rtnl_lock() before
> continuing and IOCTLs check (though devinet_ioctl()) if device is still
> registered just after taking the lock:
> 
> int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
> {
> 	// ...
> 
>         rtnl_lock();
> 
>         ret = -ENODEV;
>         dev = __dev_get_by_name(net, ifr->ifr_name);
>         if (!dev)
>                 goto done;
> 
> 	// ...
> done:
>         rtnl_unlock();
> out:
>         return ret;
> }
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@....ru>

[...]

   Sorry for overlooking this race (and other bugs) when prepping
the driver for upstream!

MBR, Sergey