[PATCH] add sysfs attribute 'carrier' for net devices

[PATCH] add sysfs attribute 'carrier' for net devices

[Jesper Juhl]

Hi,

Here's a patch that adds a new sysfs attribute for net devices. The new 
attribute 'carrier' exposes the result of netif_carrier_ok() so that when 
a network device has carrier the attribute value is 1 and when there is no 
carrier the attribute value is 0.
Very rellevant attribute for network devices in my oppinion, and sysfs is 
the logical place for it.

I've tested this only on my own machine, but I get the expected results:

With network cable plugged into eth0 : 

juhl@dragon:~$ cat /sys/class/net/eth0/carrier
1
juhl@dragon:~$

With network cable unplugged : 

juhl@dragon:~$ cat /sys/class/net/eth0/carrier
0
juhl@dragon:~$

Please review and consider applying.

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

diff -u linux-2.6.9-rc2-bk5-orig/net/core/net-sysfs.c linux-2.6.9-rc2-bk5/net/core/net-sysfs.c
--- linux-2.6.9-rc2-bk5-orig/net/core/net-sysfs.c	2004-09-14 23:19:53.000000000 +0200
+++ linux-2.6.9-rc2-bk5/net/core/net-sysfs.c	2004-09-27 00:24:01.000000000 +0200
@@ -126,8 +126,21 @@
 	return -EINVAL;
 }
 
+static ssize_t show_carrier(struct class_device *dev, char *buf)
+{
+	struct net_device *net = to_net_dev(dev);
+	if (netif_running(net)) {
+		if (netif_carrier_ok(net))
+			return snprintf(buf, 3, "%d\n", 1);
+		else
+			return snprintf(buf, 3, "%d\n", 0);
+	}
+	return -EINVAL;
+}
+
 static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
 static CLASS_DEVICE_ATTR(broadcast, S_IRUGO, show_broadcast, NULL);
+static CLASS_DEVICE_ATTR(carrier, S_IRUGO, show_carrier, NULL);
 
 /* read-write attributes */
 NETDEVICE_SHOW(mtu, fmt_dec);
@@ -186,6 +199,7 @@
 	&class_device_attr_type,
 	&class_device_attr_address,
 	&class_device_attr_broadcast,
+	&class_device_attr_carrier,
 	NULL
 };
 

Re: [PATCH] add sysfs attribute 'carrier' for net devices

[Stephen Hemminger]

On Mon, 2004-09-27 at 00:51 +0200, Jesper Juhl wrote:
> Hi,
> 
> Here's a patch that adds a new sysfs attribute for net devices. The new 
> attribute 'carrier' exposes the result of netif_carrier_ok() so that when 
> a network device has carrier the attribute value is 1 and when there is no 
> carrier the attribute value is 0.
> Very rellevant attribute for network devices in my oppinion, and sysfs is 
> the logical place for it.
> 
> I've tested this only on my own machine, but I get the expected results:
> 
> With network cable plugged into eth0 : 
> 
> juhl@dragon:~$ cat /sys/class/net/eth0/carrier
> 1
> juhl@dragon:~$
> 
> With network cable unplugged : 
> 
> juhl@dragon:~$ cat /sys/class/net/eth0/carrier
> 0
> juhl@dragon:~$
> 
> Please review and consider applying.
> 
> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> 
> diff -u linux-2.6.9-rc2-bk5-orig/net/core/net-sysfs.c linux-2.6.9-rc2-bk5/net/core/net-sysfs.c
> --- linux-2.6.9-rc2-bk5-orig/net/core/net-sysfs.c	2004-09-14 23:19:53.000000000 +0200
> +++ linux-2.6.9-rc2-bk5/net/core/net-sysfs.c	2004-09-27 00:24:01.000000000 +0200
> @@ -126,8 +126,21 @@
>  	return -EINVAL;
>  }
>  
> +static ssize_t show_carrier(struct class_device *dev, char *buf)
> +{
> +	struct net_device *net = to_net_dev(dev);
> +	if (netif_running(net)) {
> +		if (netif_carrier_ok(net))
> +			return snprintf(buf, 3, "%d\n", 1);
> +		else
> +			return snprintf(buf, 3, "%d\n", 0);

Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
The most concise format of this would be:
	return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));




> +	}
> +	return -EINVAL;
> +}
> +
>  static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
>  static CLASS_DEVICE_ATTR(broadcast, S_IRUGO, show_broadcast, NULL);
> +static CLASS_DEVICE_ATTR(carrier, S_IRUGO, show_carrier, NULL);
>  
>  /* read-write attributes */
>  NETDEVICE_SHOW(mtu, fmt_dec);
> @@ -186,6 +199,7 @@
>  	&class_device_attr_type,
>  	&class_device_attr_address,
>  	&class_device_attr_broadcast,
> +	&class_device_attr_carrier,
>  	NULL
>  };
>  

Re: [PATCH] add sysfs attribute 'carrier' for net devices

[Jesper Juhl]

On Mon, 27 Sep 2004, Stephen Hemminger wrote:

> Date: Mon, 27 Sep 2004 10:29:13 -0700
> From: Stephen Hemminger <shemminger@osdl.org>
> To: Jesper Juhl <juhl-lkml@dif.dk>
> Cc: linux-kernel <linux-kernel@vger.kernel.org>,
>     Nico Schottelius <nico-kernel@schottelius.org>
> Subject: Re: [PATCH] add sysfs attribute 'carrier' for net devices
> 
> On Mon, 2004-09-27 at 00:51 +0200, Jesper Juhl wrote:
> >  
> > +static ssize_t show_carrier(struct class_device *dev, char *buf)
> > +{
> > +	struct net_device *net = to_net_dev(dev);
> > +	if (netif_running(net)) {
> > +		if (netif_carrier_ok(net))
> > +			return snprintf(buf, 3, "%d\n", 1);
> > +		else
> > +			return snprintf(buf, 3, "%d\n", 0);
> 
> Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> The most concise format of this would be:
> 	return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
> 

I see your point and I'll create a new patch this evening when I get home 
from work.
Thank you for your feedback.


--
Jesper Juhl

Re: [PATCH] add sysfs attribute 'carrier' for net devices

[Paulo Marques]

Stephen Hemminger wrote:
> ....
>>+{
>>+	struct net_device *net = to_net_dev(dev);
>>+	if (netif_running(net)) {
>>+		if (netif_carrier_ok(net))
>>+			return snprintf(buf, 3, "%d\n", 1);
>>+		else
>>+			return snprintf(buf, 3, "%d\n", 0);
> 
> 
> Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> The most concise format of this would be:
> 	return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));

<nitpick>
Since netif_carrier_ok already has a "!" in it, it is guaranteed to 
return a 0 / 1 result. so this could be:

  	return sprintf(buf, dec_fmt, netif_carrier_ok(dev));

Of course your way is more robust to future 'netif_carrier_ok' changes 
and the compiler should optimize it way anyway since it is an inline 
function, so I actually prefer the !! version :)
</nitpick>

-- 
Paulo Marques - www.grupopie.com

To err is human, but to really foul things up requires a computer.
Farmers' Almanac, 1978

Re: [PATCH] add sysfs attribute 'carrier' for net devices

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

hi :)

On Tue, Sep 28, 2004 at 01:10:21PM +0100, Paulo Marques wrote:
> Of course your way is more robust to future 'netif_carrier_ok' changes 
> and the compiler should optimize it way anyway since it is an inline 
> function, so I actually prefer the !! version :)

But perhaps those future changes would be interesting for userspace,
too?

-- 
Martin Waitz

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] add sysfs attribute 'carrier' for net devices - try 2.

[Stephen Hemminger]

On Tue, 2004-09-28 at 21:23 +0200, Jesper Juhl wrote:
> On Tue, 28 Sep 2004, Jesper Juhl wrote:
> 
> > On Mon, 27 Sep 2004, Stephen Hemminger wrote:
> > 
> > > Date: Mon, 27 Sep 2004 10:29:13 -0700
> > > From: Stephen Hemminger <shemminger@osdl.org>
> > > To: Jesper Juhl <juhl-lkml@dif.dk>
> > > Cc: linux-kernel <linux-kernel@vger.kernel.org>,
> > >     Nico Schottelius <nico-kernel@schottelius.org>
> > > Subject: Re: [PATCH] add sysfs attribute 'carrier' for net devices
> > > 
> > > On Mon, 2004-09-27 at 00:51 +0200, Jesper Juhl wrote:
> > > >  
> > > > +static ssize_t show_carrier(struct class_device *dev, char *buf)
> > > > +{
> > > > +	struct net_device *net = to_net_dev(dev);
> > > > +	if (netif_running(net)) {
> > > > +		if (netif_carrier_ok(net))
> > > > +			return snprintf(buf, 3, "%d\n", 1);
> > > > +		else
> > > > +			return snprintf(buf, 3, "%d\n", 0);
> > > 
> > > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > > The most concise format of this would be:
> > > 	return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
> > > 
> > 
> > I see your point and I'll create a new patch this evening when I get home 
> > from work.
> > Thank you for your feedback.
> > 
> 
> Here's a second try at a patch to properly implement a 'carrier' sysfs 
> attribute for net devices.
> 
> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> 
> diff -u linux-2.6.9-rc2-bk14-orig/net/core/net-sysfs.c linux-2.6.9-rc2-bk14/net/core/net-sysfs.c
> --- linux-2.6.9-rc2-bk14-orig/net/core/net-sysfs.c	2004-09-14 23:19:53.000000000 +0200
> +++ linux-2.6.9-rc2-bk14/net/core/net-sysfs.c	2004-09-28 19:37:45.000000000 +0200
> @@ -126,8 +126,18 @@
>  	return -EINVAL;
>  }
>  
> +static ssize_t show_carrier(struct class_device *dev, char *buf)
> +{
> +	struct net_device *netdev = to_net_dev(dev);
> +	if (netif_running(netdev)) {
> +		return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev));
> +	}
> +	return -EINVAL;
> +}
> +
>  static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
>  static CLASS_DEVICE_ATTR(broadcast, S_IRUGO, show_broadcast, NULL);
> +static CLASS_DEVICE_ATTR(carrier, S_IRUGO, show_carrier, NULL);
>  
>  /* read-write attributes */
>  NETDEVICE_SHOW(mtu, fmt_dec);
> @@ -186,6 +196,7 @@
>  	&class_device_attr_type,
>  	&class_device_attr_address,
>  	&class_device_attr_broadcast,
> +	&class_device_attr_carrier,
>  	NULL
>  };

Looks great, it kind of shows how easy sysfs is but how baroque it is as
well.

[PATCH] add sysfs attribute 'carrier' for net devices - try 2.

[Jesper Juhl]

On Tue, 28 Sep 2004, Jesper Juhl wrote:

> On Mon, 27 Sep 2004, Stephen Hemminger wrote:
> 
> > Date: Mon, 27 Sep 2004 10:29:13 -0700
> > From: Stephen Hemminger <shemminger@osdl.org>
> > To: Jesper Juhl <juhl-lkml@dif.dk>
> > Cc: linux-kernel <linux-kernel@vger.kernel.org>,
> >     Nico Schottelius <nico-kernel@schottelius.org>
> > Subject: Re: [PATCH] add sysfs attribute 'carrier' for net devices
> > 
> > On Mon, 2004-09-27 at 00:51 +0200, Jesper Juhl wrote:
> > >  
> > > +static ssize_t show_carrier(struct class_device *dev, char *buf)
> > > +{
> > > +	struct net_device *net = to_net_dev(dev);
> > > +	if (netif_running(net)) {
> > > +		if (netif_carrier_ok(net))
> > > +			return snprintf(buf, 3, "%d\n", 1);
> > > +		else
> > > +			return snprintf(buf, 3, "%d\n", 0);
> > 
> > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > The most concise format of this would be:
> > 	return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
> > 
> 
> I see your point and I'll create a new patch this evening when I get home 
> from work.
> Thank you for your feedback.
> 

Here's a second try at a patch to properly implement a 'carrier' sysfs 
attribute for net devices.

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

diff -u linux-2.6.9-rc2-bk14-orig/net/core/net-sysfs.c linux-2.6.9-rc2-bk14/net/core/net-sysfs.c
--- linux-2.6.9-rc2-bk14-orig/net/core/net-sysfs.c	2004-09-14 23:19:53.000000000 +0200
+++ linux-2.6.9-rc2-bk14/net/core/net-sysfs.c	2004-09-28 19:37:45.000000000 +0200
@@ -126,8 +126,18 @@
 	return -EINVAL;
 }
 
+static ssize_t show_carrier(struct class_device *dev, char *buf)
+{
+	struct net_device *netdev = to_net_dev(dev);
+	if (netif_running(netdev)) {
+		return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev));
+	}
+	return -EINVAL;
+}
+
 static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
 static CLASS_DEVICE_ATTR(broadcast, S_IRUGO, show_broadcast, NULL);
+static CLASS_DEVICE_ATTR(carrier, S_IRUGO, show_carrier, NULL);
 
 /* read-write attributes */
 NETDEVICE_SHOW(mtu, fmt_dec);
@@ -186,6 +196,7 @@
 	&class_device_attr_type,
 	&class_device_attr_address,
 	&class_device_attr_broadcast,
+	&class_device_attr_carrier,
 	NULL
 };
 

Re: [PATCH] add sysfs attribute 'carrier' for net devices - try 2.

[Nico Schottelius]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

Hello!

I am just a bit tired and a bit confused, but

Jesper Juhl [Tue, Sep 28, 2004 at 09:23:25PM +0200]:
> > > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > > The most concise format of this would be:
> > > 	return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));

wouldn't this potentially open a security hole / buffer overflow?

I am currently not really seeing where buf is passed from and what
dec_fmt is, but am I totally wrong?

Nico

-- 
Keep it simple & stupid, use what's available.
Please use pgp encryption: 8D0E 27A4 is my id.
http://nico.schotteli.us | http://linux.schottelius.org

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH] add sysfs attribute 'carrier' for net devices - try 2.

[Stephen Hemminger]

On Tue, 2004-09-28 at 22:54 +0200, Nico Schottelius wrote:
> Hello!
> 
> I am just a bit tired and a bit confused, but
> 
> Jesper Juhl [Tue, Sep 28, 2004 at 09:23:25PM +0200]:
> > > > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > > > The most concise format of this would be:
> > > > 	return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
> 
> wouldn't this potentially open a security hole / buffer overflow?

buf comes from fs/sysfs/file.c:fill_read_buf and is PAGESIZE.
Since netif_carrier_ok() returns 0 or 1, don't see how that could ever
turn into a security hole.
	

> I am currently not really seeing where buf is passed from and what
> dec_fmt is, but am I totally wrong?

dec_fmt is in net-sysfs.c and is defined as "%d\n" to avoid
repetition of same string literal.

Re: [PATCH] add sysfs attribute 'carrier' for net devices - try 2.

[Nico Schottelius]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

Thanks for enlightening [1] me!

Nico

P.S.: [1] enlightenment.org is still alive, just wanted to note that..

Stephen Hemminger [Tue, Sep 28, 2004 at 03:24:43PM -0700]:
> On Tue, 2004-09-28 at 22:54 +0200, Nico Schottelius wrote:
> > Hello!
> > 
> > I am just a bit tired and a bit confused, but
> > 
> > Jesper Juhl [Tue, Sep 28, 2004 at 09:23:25PM +0200]:
> > > > > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > > > > The most concise format of this would be:
> > > > > 	return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
> > 
> > wouldn't this potentially open a security hole / buffer overflow?
> 
> buf comes from fs/sysfs/file.c:fill_read_buf and is PAGESIZE.
> Since netif_carrier_ok() returns 0 or 1, don't see how that could ever
> turn into a security hole.
> 	
> 
> > I am currently not really seeing where buf is passed from and what
> > dec_fmt is, but am I totally wrong?
> 
> dec_fmt is in net-sysfs.c and is defined as "%d\n" to avoid
> repetition of same string literal.
> 
> 

-- 
Keep it simple & stupid, use what's available.
Please use pgp encryption: 8D0E 27A4 is my id.
http://nico.schotteli.us | http://linux.schottelius.org

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH] add sysfs attribute 'carrier' for net devices

[Nico Schottelius]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

> On Fri, 22 Oct 2004, Jesper Juhl wrote:
> [sysfs kernel patch]
> Good news, 
> Linus has merged the patch in 2.6.10-rc1 so unless someone finds a flaw 
> with it or otherwise complains about it it should be in 2.6.10 once that 
> is released.

I tested it in 2.6.9 and it works without any problems. I even
included it in an isntallation script and tested pluggin cable
out and in, works like charme.

Nico

-- 
Keep it simple & stupid, use what's available.
Please use pgp encryption: 8D0E 27A4 is my id.
http://nico.schotteli.us | http://linux.schottelius.org

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]