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

Message-Id: <20170824.121559.882635807428126397.davem@davemloft.net>
Date:   Thu, 24 Aug 2017 12:15:59 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     subashab@...eaurora.org
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

From: Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
Date: Mon, 21 Aug 2017 16:36:59 -0600

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/Makefile b/drivers/net/ethernet/qualcomm/rmnet/Makefile
> new file mode 100644
> index 0000000..1c43e2f
> --- /dev/null
> +++ b/drivers/net/ethernet/qualcomm/rmnet/Makefile
> @@ -0,0 +1,12 @@
> +#
> +# Makefile for the RMNET module
> +#
> +
> +rmnet-y		 := rmnet_config.o
> +rmnet-y		 += rmnet_vnd.o
> +rmnet-y		 += rmnet_handlers.o
> +rmnet-y		 += rmnet_map_data.o
> +rmnet-y		 += rmnet_map_command.o
> +obj-$(CONFIG_RMNET) += rmnet.o
> +
> +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.

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

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.