Re: [Patch net] net: fix dev_ifsioc_locked() race condition

On Thu, Jan 28, 2021 at 9:21 PM Jakub Kicinski [off-list ref] wrote:

On Thu, 28 Jan 2021 21:08:05 -0800 Cong Wang wrote:

quoted

On Thu, Jan 28, 2021 at 12:55 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Sat, 23 Jan 2021 17:30:49 -0800 Cong Wang wrote:

quoted

From: Cong Wang <redacted>

dev_ifsioc_locked() is called with only RCU read lock, so when
there is a parallel writer changing the mac address, it could
get a partially updated mac address, as shown below:

Thread 1                      Thread 2
// eth_commit_mac_addr_change()
memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
                              // dev_ifsioc_locked()
                              memcpy(ifr->ifr_hwaddr.sa_data,
                                      dev->dev_addr,...);

Close this race condition by guarding them with a RW semaphore,
like netdev_get_name(). The writers take RTNL anyway, so this
will not affect the slow path.

Fixes: 3710becf8a58 ("net: RCU locking for simple ioctl()")
Reported-by: "Gong, Sishuai" <redacted>
Cc: Eric Dumazet <redacted>
Signed-off-by: Cong Wang <redacted>

The addition of the write lock scares me a little for a fix, there's a
lot of code which can potentially run under the callbacks and notifiers
there.

What about using a seqlock?

Actually I did use seqlock in my initial version (not posted), it does not
allow blocking inside write_seqlock() protection, so I have to change
to rwsem.

Argh, you're right. No way we can construct something that tries to
read once and if it fails falls back to waiting for RTNL?

I don't think there is any way to tell whether the read fails, a partially
updated address can not be detected without additional flags etc..

And devnet_rename_sem is already there, pretty much similar to this
one.

Thanks.