netdev - Re: [RFC PATCH v6 06/11] leds: trigger: netdev: add hardware control support

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: <YnMhk1F0LrIMK5hp@lunn.ch>
Date:   Thu, 5 May 2022 03:00:03 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Ansuel Smith <ansuelsmth@...il.com>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Jonathan Corbet <corbet@....net>, Pavel Machek <pavel@....cz>,
        John Crispin <john@...ozen.org>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [RFC PATCH v6 06/11] leds: trigger: netdev: add hardware control
 support

> +struct netdev_led_attr_detail {
> +	char *name;
> +	bool hardware_only;
> +	enum led_trigger_netdev_modes bit;
> +};
> +
> +static struct netdev_led_attr_detail attr_details[] = {
> +	{ .name = "link", .bit = TRIGGER_NETDEV_LINK},
> +	{ .name = "tx", .bit = TRIGGER_NETDEV_TX},
> +	{ .name = "rx", .bit = TRIGGER_NETDEV_RX},

hardware_only is never set. Maybe it is used in a later patch? If so,
please introduce it there.

>  static void set_baseline_state(struct led_netdev_data *trigger_data)
>  {
> +	int i;
>  	int current_brightness;
> +	struct netdev_led_attr_detail *detail;
>  	struct led_classdev *led_cdev = trigger_data->led_cdev;

This file mostly keeps with reverse christmas tree, probably because
it was written by a netdev developer. It is probably not required for
the LED subsystem, but it would be nice to keep the file consistent.

> @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
>  				 size_t size)
>  {
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> +	struct net_device *old_net = trigger_data->net_dev;
> +	char old_device_name[IFNAMSIZ];
>  
>  	if (size >= IFNAMSIZ)
>  		return -EINVAL;
>  
> +	/* Backup old device name */
> +	memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> +
>  	cancel_delayed_work_sync(&trigger_data->work);
>  
>  	spin_lock_bh(&trigger_data->lock);
> @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
>  		trigger_data->net_dev =
>  		    dev_get_by_name(&init_net, trigger_data->device_name);
>  
> +	if (!validate_baseline_state(trigger_data)) {

You probably want to validate trigger_data->net_dev is not NULL first. The current code
is a little odd with that, 

> +		/* Restore old net_dev and device_name */
> +		if (trigger_data->net_dev)
> +			dev_put(trigger_data->net_dev);
> +
> +		dev_hold(old_net);

This dev_hold() looks wrong. It is trying to undo a dev_put()
somewhere? You should not actually do a put until you know you really
do not old_net, otherwise there is a danger it disappears and you
cannot undo.

> @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
>  		return ret;
>  
>  	/* impose some basic bounds on the timer interval */
> -	if (value >= 5 && value <= 10000) {
> -		cancel_delayed_work_sync(&trigger_data->work);
> +	if (value < 5 || value > 10000)
> +		return -EINVAL;
> +
> +	cancel_delayed_work_sync(&trigger_data->work);
> +
> +	atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
>  
> -		atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> -		set_baseline_state(trigger_data);	/* resets timer */
> +	if (!validate_baseline_state(trigger_data)) {
> +		/* Restore old interval on validation error */
> +		atomic_set(&trigger_data->interval, old_interval);
> +		trigger_data->mode = old_mode;

I think you need to schedule the work again, since you cancelled
it. It is at the end of the work that the next work is scheduled, and
so it will not self recover.

   Andrew