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

- From: Subash Abhinov Kasiviswanathan \<hidden\>
- Date: 2017-08-24 22:45:31 
- List: netdev
- Page: https://ratatoskr.run/netdev/2017/08/11064033
- Thread: https://ratatoskr.run/netdev/2017/08/11063620/t.md

```text
>> +
>> +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.
```