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

On Fri, Jan 29, 2021 at 10:00 AM Jakub Kicinski [off-list ref] wrote:

On Thu, 28 Jan 2021 21:47:04 -0800 Cong Wang wrote:

quoted

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

quoted

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..

Let me pseudo code it, I can't English that well:

void reader(obj)
{
        unsigned int seq;

        seq = READ_ONCE(seqcnt);
        if (seq & 1)
                goto slow_path;
        smb_rmb();

        obj = read_the_thing();

        smb_rmb();
        if (seq == READ_ONCE(seqcnt))
                return;

slow_path:
        rtnl_lock();
        obj = read_the_thing();
        rtnl_unlock();
}

void writer()
{
        ASSERT_RNTL();

        seqcnt++;
        smb_wmb();

        modify_the_thing();

        smb_wmb();
        seqcnt++;
}


I think raw_seqcount helpers should do here?

Not sure why you want to kinda rewrite seqlock here. Please see below.

quoted

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

Ack, I don't see rename triggering cascading notifications, tho.
I think you've seen the recent patch for locking in team, that's
pretty much what I'm afraid will happen here.

Right, I should make only user-facing callers (ioctl, rtnetlink) use this
writer semaphore, and leave other callers of dev_set_mac_address()
untouched. Something like:

dev_set_mac_address_locked(...)
{
  down_write(&dev_addr_sem);
  dev_set_mac_address(...);
  up_write(&dev_addr_sem);
  ...
}

With this, we don't need to reinvent seqlock.

But if I'm missing something about the seqcount or you strongly prefer
the rwlock, we can do that, too. Although I'd rather take this patch
to net-next in that case.

I have no problem with applying to net-next. I will send v2 targeting
net-next instead.

Thanks.