netdev - Re: [PATCH net-next 3/3 v4] 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]

Date:   Tue, 15 Aug 2017 21:24:14 -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
Subject: Re: [PATCH net-next 3/3 v4] drivers: net: ethernet: qualcomm:
 rmnet: Initial implementation

From: Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
Date: Tue, 15 Aug 2017 22:15:53 -0600

> +static int rmnet_unregister_real_device(struct net_device *dev)
> +{
> +	int config_id = RMNET_LOCAL_LOGICAL_ENDPOINT;
> +	struct rmnet_logical_ep_conf_s *epconfig_l;
> +	struct rmnet_phys_ep_conf_s *config;
> +
> +	ASSERT_RTNL();
> +
> +	netdev_info(dev, "Removing device %s\n", dev->name);
> +
> +	if (!rmnet_is_real_dev_registered(dev))
> +		return -EINVAL;
> +
> +	for (; config_id < RMNET_MAX_LOGICAL_EP; config_id++) {

This loop is so much harder to understand because you initialize
the loop index several lines above the for() statement.  Just
initialize it here at the beginning of the for() loop and deal
with the fact that this will have to therefore be a multi-line
for() statement the best you can.

> +static int rmnet_set_egress_data_format(struct net_device *dev,	u32 edf,
> +					u16 agg_size, u16 agg_count)

Use a space instead of a TAB character before the "u32 edf," argument.

> +static int
> +__rmnet_set_logical_endpoint_config(struct net_device *dev,
> +				    int config_id,
> +				    struct rmnet_logical_ep_conf_s *epconfig)
> +{
> +	struct rmnet_logical_ep_conf_s *epconfig_l;
> +
> +	ASSERT_RTNL();
> +
> +	epconfig_l = rmnet_get_logical_ep(dev, config_id);
> +
> +	if (!epconfig_l || epconfig_l->refcount)
> +		return -EINVAL;
> +
> +	memcpy(epconfig_l, epconfig, sizeof(struct rmnet_logical_ep_conf_s));
> +	if (config_id == RMNET_LOCAL_LOGICAL_ENDPOINT)
> +		epconfig_l->mux_id = 0;
> +	else
> +		epconfig_l->mux_id = config_id;
> +
> +	dev_hold(epconfig_l->egress_dev);
> +	return 0;
> +}

This looks really messy.

One invariant that must hold is that if I try to take down netdev
"X", all resources holding a reference to X will be immediately
(or quickly) dropped when that request comes in via notifiers.

Will this happen properly for this egress_dev reference counting?

> +static int rmnet_config_notify_cb(struct notifier_block *nb,
> +				  unsigned long event, void *data)
> +{
> +	struct net_device *dev = netdev_notifier_info_to_dev(data);
> +
> +	if (!dev)
> +		return NOTIFY_DONE;
> +
> +	switch (event) {
> +	case NETDEV_UNREGISTER_FINAL:
> +	case NETDEV_UNREGISTER:
> +		netdev_info(dev, "Kernel unregister\n");
> +		rmnet_force_unassociate_device(dev);
> +		break;

So I guess it happens here?