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

quoted

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

quoted

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