af_packet.c bug?

af_packet.c bug?

[Ben Greear]

What is the '13' doing here?  Maybe it should be IFNAMSIZ?

	/*
	 *	Find the device first to size check it
	 */

	saddr->spkt_device[13] = 0;
	dev = dev_get_by_name(saddr->spkt_device);
	err = -ENODEV;
	if (dev == NULL)
		goto out_unlock;



-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: af_packet.c bug?

[Thomas Graf]

* Ben Greear <424858D4.8060604@candelatech.com> 2005-03-28 11:19
> 
> What is the '13' doing here?  Maybe it should be IFNAMSIZ?
> 
> 	/*
> 	 *	Find the device first to size check it
> 	 */
> 
> 	saddr->spkt_device[13] = 0;
> 	dev = dev_get_by_name(saddr->spkt_device);
> 	err = -ENODEV;
> 	if (dev == NULL)
> 		goto out_unlock;

Seems so, please adopt the size of spkt_device in struct sockaddr_pkt
as well if you change it, it's currently hardcoded as 14.

Re: af_packet.c bug?

[Ben Greear]

Thomas Graf wrote:
> * Ben Greear <424858D4.8060604@candelatech.com> 2005-03-28 11:19
> 
>>What is the '13' doing here?  Maybe it should be IFNAMSIZ?
>>
>>	/*
>>	 *	Find the device first to size check it
>>	 */
>>
>>	saddr->spkt_device[13] = 0;
>>	dev = dev_get_by_name(saddr->spkt_device);
>>	err = -ENODEV;
>>	if (dev == NULL)
>>		goto out_unlock;
> 
> 
> Seems so, please adopt the size of spkt_device in struct sockaddr_pkt

you mean adapt maybe?

> as well if you change it, it's currently hardcoded as 14.

I was also wondering why we couldn't hold a reference to the net-device
instead of just it's ifindex when dealing with a bound raw socket.

That would save the device lookup for each packet sent.

I figure that we would need to listen for NETDEV_UNREGISTER
events and do a dev_put whenever the device wants to go away.

Does that sound workable?

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: af_packet.c bug?

[Thomas Graf]

* Ben Greear <4248642E.40304@candelatech.com> 2005-03-28 12:08
> >Seems so, please adopt the size of spkt_device in struct sockaddr_pkt
> 
> you mean adapt maybe?

Of course, stupid typo. ;->

> I was also wondering why we couldn't hold a reference to the net-device
> instead of just it's ifindex when dealing with a bound raw socket.

Saving a netdevice handle in the packet socket is not a problem but
doing the same for the packet socket address seems to be non trivial.

Re: af_packet.c bug?

[David S. Miller]

On Mon, 28 Mar 2005 12:08:14 -0800
Ben Greear <greearb@candelatech.com> wrote:

> I was also wondering why we couldn't hold a reference to the net-device
> instead of just it's ifindex when dealing with a bound raw socket.

Because then raw sockets could make unloading of netdevices
hang forever.  That is, unless you add some netdev notifier
to af_packet.c that walks all the raw sockets looking for netdev
references.

I guess it also would make things like leaving a dump running
while you quickly down/up and interface stop working.

Re: af_packet.c bug?

[Ben Greear]

David S. Miller wrote:
> On Mon, 28 Mar 2005 12:08:14 -0800
> Ben Greear <greearb@candelatech.com> wrote:
> 
> 
>>I was also wondering why we couldn't hold a reference to the net-device
>>instead of just it's ifindex when dealing with a bound raw socket.
> 
> 
> Because then raw sockets could make unloading of netdevices
> hang forever.  That is, unless you add some netdev notifier
> to af_packet.c that walks all the raw sockets looking for netdev
> references.

 > I guess it also would make things like leaving a dump running
 > while you quickly down/up and interface stop working.
 >

I was thinking of removing the reference only when the NETDEV_UNREGISTER
event was sent.  That will allow sockets to work through UP and DOWN transitions
of the device as it does currently.

We could also save the name of the interface in the socket struct and resolve
the device in a lazy manner.  That way, even if the device is truly gone, the
worst case performance will be like it is today.

It would be nice to have a list of all of these sockets so we could get info
on them similar to the 'netstat -an' output anyway...

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: af_packet.c bug?

[David S. Miller]

On Mon, 28 Mar 2005 12:30:25 -0800
Ben Greear <greearb@candelatech.com> wrote:

> It would be nice to have a list of all of these sockets so we could get info
> on them similar to the 'netstat -an' output anyway...

/proc/net/packet is insufficient?

Re: af_packet.c bug?

[Ben Greear]

Thomas Graf wrote:
> * Ben Greear <4248642E.40304@candelatech.com> 2005-03-28 12:08
> 
>>>Seems so, please adopt the size of spkt_device in struct sockaddr_pkt
>>
>>you mean adapt maybe?
> 
> 
> Of course, stupid typo. ;->
> 
>>I was also wondering why we couldn't hold a reference to the net-device
>>instead of just it's ifindex when dealing with a bound raw socket.
> 
> 
> Saving a netdevice handle in the packet socket is not a problem but
> doing the same for the packet socket address seems to be non trivial.

Yes.  But assuming you bind the socket, you use the device saved in the packet
socket.  If you are sending to a particular device for each send(), then you'll
just have to pay the lookup costs.  I haven't looked recently, but didn't someone
finally hash the device lookup so that it's not a linear walk of the device
list now?

Ben



-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: af_packet.c bug?

[David S. Miller]

On Mon, 28 Mar 2005 21:55:57 +0200
Thomas Graf <tgraf@suug.ch> wrote:

> * Ben Greear <424858D4.8060604@candelatech.com> 2005-03-28 11:19
> > 
> > What is the '13' doing here?  Maybe it should be IFNAMSIZ?
> > 
> > 	/*
> > 	 *	Find the device first to size check it
> > 	 */
> > 
> > 	saddr->spkt_device[13] = 0;
> > 	dev = dev_get_by_name(saddr->spkt_device);
> > 	err = -ENODEV;
> > 	if (dev == NULL)
> > 		goto out_unlock;
> 
> Seems so, please adopt the size of spkt_device in struct sockaddr_pkt
> as well if you change it, it's currently hardcoded as 14.

No can do, that structure is exported to userspace.
Thus, changing it from 14 to IFNAMESZ (which is 16) will
break stuff.