netdev - Re: [PATCH net-next 3/3 v7] drivers: net: ethernet: qualcomm: rmnet: Initial implementation

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] [day] [month] [year] [list]

Message-ID: <0a82b4c5349c3201c58a13f90b5ea758@codeaurora.org>
Date:   Thu, 24 Aug 2017 16:45:30 -0600
From:   Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, fengguang.wu@...el.com, dcbw@...hat.com,
        jiri@...nulli.us, stephen@...workplumber.org,
        David.Laight@...lab.com, marcel@...tmann.org
Subject: Re: [PATCH net-next 3/3 v7] drivers: net: ethernet: qualcomm: rmnet:
 Initial implementation

>> +
>> +CFLAGS_rmnet.o := -I$(src)
> 
> You do not need this CFLAGS rule, the local include files are included
> using "" double quotes so it uses the local directory always.

Hi David

I'll remove this.

>> +static void rmnet_free_later(struct work_struct *work)
>> +{
>> +	struct rmnet_free_work *fwork;
>> +
>> +	fwork = container_of(work, struct rmnet_free_work, work);
>> +
>> +	rtnl_lock();
>> +	rmnet_delink(fwork->rmnet_dev, NULL);
>> +	rtnl_unlock();
>> +
>> +	kfree(fwork);
>> +}
> 
> This is racy and doesn't work properly.
> 
> When you schedule this work, the RTNL mutex is dropped.  Meanwhile
> another request can come in the associate this device.
> 
> Your work function will still run and erroneously unlink the object.
> 
> Furthermore, during this time that the RTNL mutex is dropped, people
> will see the unassociated device in the lists.
> 
> You have to atomically remove the object from all possible locations
> which provide external visibility of that object, before the RTNL
> mutex is dropped.
> 
> So you can defer the freeing, but you cannot defer the unlink
> operation.

I had incorrectly assumed earlier that the check in rtnl_newlink for
NLM_F_EXCL would guard against the scenario of re-associating a
device which was unlinked.

> 
> You probably need to move to RCU as well in order for all of this to
> work properly since scans of the lists occur in the data path which
> is completely asynchronous and not protected by the RTNL mutex.

I'll remove all the rtnl locks and checks and switch to rcu and post
an update.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project