[uml-devel] When opening /tmp/uml.ctl fails, what do we get?

[uml-devel] When opening /tmp/uml.ctl fails, what do we get?

[Blaisorblade]

I started uml_daemon as root by mistake, so /tmp/uml.ctl even if /dev/net/tun 
was world-readable. Ok, I did it. This is the result (after trying many times 
to do 'ifconfig eth0 up' inside UML):

[42949710.290000] Choosing a random ethernet address for device eth0
[42949710.290000] Netdevice 0 (3a:9d:6f:12:83:b2) : daemon backend (uml_switch 
version 3) - unix:/tmp/uml.ctl
[42949710.290000] daemon_open : control connect failed, errno = 13
(13 = EPERM)
[42949720.360000] Registering fd 0 twice
[42949720.360000] Irqs : 2, 5
[42949720.360000] Ids : 0x0000000061a795f8, 0x0000000061d181c0
[42949720.360000] uml_net_open: failed to get irq(-12)

(-12 is -ENOMEM, not -13 + 1, but is caused by 'registering XXX twice').

[42949720.360000] Registering fd 0 twice
[42949720.360000] Irqs : 2, 5
[42949720.360000] Ids : 0x0000000061a795f8, 0x0000000061d181c0
[42949720.360000] uml_net_open: failed to get irq(-12)

Looking into the code, I saw that:
1) daemon_user_init is confusing because it saved any error into pri->fd, 
instead of returning it to the caller. Also, it would make sense to move this 
into daemon_open.
2) connect_to_switch uses the same errno twice - it shouldn't, and we're 
hitting this. um_interrupt is called with fd => 0 because errno is cleared 
after printk (verified under debugging, I did not trace it fully to where 
it's changing but it's sufficient for a patch anyway).
-- 
Inform me of my mistakes, so I can add them to my list!
Paolo Giarrusso, aka Blaisorblade
http://www.user-mode-linux.org/~blaisorblade
Chiacchiera con i tuoi amici in tempo reale! 
 http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com 


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] When opening /tmp/uml.ctl fails, what do we get?

[Jeff Dike]

On Thu, Feb 22, 2007 at 12:54:04AM +0100, Blaisorblade wrote:
> Looking into the code, I saw that:
> 1) daemon_user_init is confusing because it saved any error into pri->fd, 
> instead of returning it to the caller. 

Yup.

> Also, it would make sense to move this into daemon_open.

The current behavior is on purpose.  The thinking is that device
initialization is equivalent to plugging a box into a switch.  The
device isn't up (and the box might in fact be turned off).  Bringing
the interface up comes later, as a separate step.

You may be right, and we can talk about it, but the current code is no
accident.

> 2) connect_to_switch uses the same errno twice - it shouldn't, and we're 
> hitting this. um_interrupt is called with fd => 0 because errno is cleared 
> after printk (verified under debugging, I did not trace it fully to where 
> it's changing but it's sufficient for a patch anyway).

We've seen this before, and I thought I made a pass over all of UML
and fixed everything.  Sigh.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] When opening /tmp/uml.ctl fails, what do we get?

[Jeff Dike]

On Thu, Feb 22, 2007 at 12:54:04AM +0100, Blaisorblade wrote:
> I started uml_daemon as root by mistake, so /tmp/uml.ctl even if /dev/net/tun
> was world-readable. Ok, I did it. This is the result (after trying many times
> to do 'ifconfig eth0 up' inside UML):

I don't see your irq stuff here when I try that, but there were
multiple problems, which I hope are fixed by the patch below:
	style violations - this made the patch much bigger, and needs
to be split out
	the user init routines now return success or failure
	some major cleaning of the failure paths through eth_configure
		device was never freed
		sysfs_unregister was never called, adding the same
interface again successfully would result in nasty error messages
		device stuck in the devices list regardless, so adding
the same interface again would fail
		the return from platform_device_register was not
checked
		I moved the register_netdev call until almost the end
of the function
	

Let me know what you think about this.

And about your thought of merging the _init into _open, my excuse for
keeping them separate deserves some scrutiny.  Every transport besides
the switch does all of its work in _open, and the _init routine just
initializes data.  So, I would say that if a transport has separate
plug-it-in and bring-it-up operations, maybe they should be separated
more than they are now.  If the switch is the odd one, maybe we should
merge _init into _open.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

Index: linux-2.6.18-mm/arch/um/drivers/daemon_user.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/drivers/daemon_user.c	2007-02-22 12:54:02.000000000 -0500
+++ linux-2.6.18-mm/arch/um/drivers/daemon_user.c	2007-02-22 13:44:09.000000000 -0500
@@ -39,7 +39,7 @@ static struct sockaddr_un *new_addr(void
 	sun = um_kmalloc(sizeof(struct sockaddr_un));
 	if(sun == NULL){
 		printk("new_addr: allocation of sockaddr_un failed\n");
-		return(NULL);
+		return NULL;
 	}
 	sun->sun_family = AF_UNIX;
 	memcpy(sun->sun_path, name, len);
@@ -122,7 +122,7 @@ static int connect_to_switch(struct daem
 	return err;
 }
 
-static void daemon_user_init(void *data, void *dev)
+static int daemon_user_init(void *data, void *dev)
 {
 	struct daemon_data *pri = data;
 	struct timeval tv;
@@ -145,13 +145,16 @@ static void daemon_user_init(void *data,
 	if(pri->fd < 0){
 		kfree(pri->local_addr);
 		pri->local_addr = NULL;
+		return pri->fd;
 	}
+
+	return 0;
 }
 
 static int daemon_open(void *data)
 {
 	struct daemon_data *pri = data;
-	return(pri->fd);
+	return pri->fd;
 }
 
 static void daemon_remove(void *data)
@@ -175,12 +178,12 @@ int daemon_user_write(int fd, void *buf,
 {
 	struct sockaddr_un *data_addr = pri->data_addr;
 
-	return(net_sendto(fd, buf, len, data_addr, sizeof(*data_addr)));
+	return net_sendto(fd, buf, len, data_addr, sizeof(*data_addr));
 }
 
 static int daemon_set_mtu(int mtu, void *data)
 {
-	return(mtu);
+	return mtu;
 }
 
 const struct net_user_info daemon_user_info = {
@@ -193,14 +196,3 @@ const struct net_user_info daemon_user_i
 	.delete_address = NULL,
 	.max_packet	= MAX_PACKET - ETH_HEADER_OTHER
 };
-
-/*
- * Overrides for Emacs so that we follow Linus's tabbing style.
- * Emacs will notice this stuff at the end of the file and automatically
- * adjust the settings for this buffer only.  This must remain at the end
- * of the file.
- * ---------------------------------------------------------------------------
- * Local variables:
- * c-file-style: "linux"
- * End:
- */
Index: linux-2.6.18-mm/arch/um/drivers/mcast_user.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/drivers/mcast_user.c	2006-12-29 12:20:14.000000000 -0500
+++ linux-2.6.18-mm/arch/um/drivers/mcast_user.c	2007-02-22 13:45:00.000000000 -0500
@@ -39,15 +39,16 @@ static struct sockaddr_in *new_addr(char
 	sin->sin_family = AF_INET;
 	sin->sin_addr.s_addr = in_aton(addr);
 	sin->sin_port = htons(port);
-	return(sin);
+	return sin;
 }
 
-static void mcast_user_init(void *data, void *dev)
+static int mcast_user_init(void *data, void *dev)
 {
 	struct mcast_data *pri = data;
 
 	pri->mcast_addr = new_addr(pri->addr, pri->port);
 	pri->dev = dev;
+	return 0;
 }
 
 static int mcast_open(void *data)
@@ -145,12 +146,12 @@ int mcast_user_write(int fd, void *buf, 
 {
 	struct sockaddr_in *data_addr = pri->mcast_addr;
 
-	return(net_sendto(fd, buf, len, data_addr, sizeof(*data_addr)));
+	return net_sendto(fd, buf, len, data_addr, sizeof(*data_addr));
 }
 
 static int mcast_set_mtu(int mtu, void *data)
 {
-	return(mtu);
+	return mtu;
 }
 
 const struct net_user_info mcast_user_info = {
Index: linux-2.6.18-mm/arch/um/drivers/net_kern.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/drivers/net_kern.c	2007-02-19 11:52:41.000000000 -0500
+++ linux-2.6.18-mm/arch/um/drivers/net_kern.c	2007-02-22 14:53:00.000000000 -0500
@@ -325,30 +325,31 @@ static struct platform_driver uml_net_dr
 };
 static int driver_registered;
 
-static int eth_configure(int n, void *init, char *mac,
-			 struct transport *transport)
+static void eth_configure(int n, void *init, char *mac,
+			  struct transport *transport)
 {
 	struct uml_net *device;
 	struct net_device *dev;
 	struct uml_net_private *lp;
 	int save, err, size;
 
-	size = transport->private_size + sizeof(struct uml_net_private) +
-		sizeof(((struct uml_net_private *) 0)->user);
+	/*
+	 * This overallocates by sizeof(int) since the last field in
+	 * uml_net_private is used to access the transport-specific
+	 * data, so that user field is the first 4 bytes of the
+	 * transport data.
+	 */
+	size = transport->private_size + sizeof(struct uml_net_private);
 
 	device = kzalloc(sizeof(*device), GFP_KERNEL);
 	if (device == NULL) {
 		printk(KERN_ERR "eth_configure failed to allocate uml_net\n");
-		return(1);
+		return;
 	}
 
 	INIT_LIST_HEAD(&device->list);
 	device->index = n;
 
-	spin_lock(&devices_lock);
-	list_add(&device->list, &devices);
-	spin_unlock(&devices_lock);
-
 	setup_etheraddr(mac, device->mac);
 
 	printk(KERN_INFO "Netdevice %d ", n);
@@ -360,7 +361,7 @@ static int eth_configure(int n, void *in
 	dev = alloc_etherdev(size);
 	if (dev == NULL) {
 		printk(KERN_ERR "eth_configure: failed to allocate device\n");
-		return 1;
+		goto out_free_device;
 	}
 
 	lp = dev->priv;
@@ -376,7 +377,8 @@ static int eth_configure(int n, void *in
 	}
 	device->pdev.id = n;
 	device->pdev.name = DRIVER_NAME;
-	platform_device_register(&device->pdev);
+	if(platform_device_register(&device->pdev))
+		goto out_free_netdev;
 	SET_NETDEV_DEV(dev,&device->pdev.dev);
 
 	/* If this name ends up conflicting with an existing registered
@@ -386,31 +388,11 @@ static int eth_configure(int n, void *in
 	snprintf(dev->name, sizeof(dev->name), "eth%d", n);
 	device->dev = dev;
 
+	/* These just fill in a data structure, so there's no failure
+	 * to be worried about.
+	 */
 	(*transport->kern->init)(dev, init);
 
-	dev->mtu = transport->user->max_packet;
-	dev->open = uml_net_open;
-	dev->hard_start_xmit = uml_net_start_xmit;
-	dev->stop = uml_net_close;
-	dev->get_stats = uml_net_get_stats;
-	dev->set_multicast_list = uml_net_set_multicast_list;
-	dev->tx_timeout = uml_net_tx_timeout;
-	dev->set_mac_address = uml_net_set_mac;
-	dev->change_mtu = uml_net_change_mtu;
-	dev->ethtool_ops = &uml_net_ethtool_ops;
-	dev->watchdog_timeo = (HZ >> 1);
-	dev->irq = UM_ETH_IRQ;
-
-	rtnl_lock();
-	err = register_netdevice(dev);
-	rtnl_unlock();
-	if (err) {
-		device->dev = NULL;
-		/* XXX: should we call ->remove() here? */
-		free_netdev(dev);
-		return 1;
-	}
-
 	/* lp.user is the first four bytes of the transport data, which
 	 * has already been initialized.  This structure assignment will
 	 * overwrite that, so we make sure that .user gets overwritten with
@@ -438,12 +420,45 @@ static int eth_configure(int n, void *in
 	lp->tl.function = uml_net_user_timer_expire;
 	memcpy(lp->mac, device->mac, sizeof(lp->mac));
 
-	if (transport->user->init)
-		(*transport->user->init)(&lp->user, dev);
+	if ((transport->user->init != NULL) &&
+	    ((*transport->user->init)(&lp->user, dev) != 0))
+		goto out_unregister;
 
 	set_ether_mac(dev, device->mac);
+	dev->mtu = transport->user->max_packet;
+	dev->open = uml_net_open;
+	dev->hard_start_xmit = uml_net_start_xmit;
+	dev->stop = uml_net_close;
+	dev->get_stats = uml_net_get_stats;
+	dev->set_multicast_list = uml_net_set_multicast_list;
+	dev->tx_timeout = uml_net_tx_timeout;
+	dev->set_mac_address = uml_net_set_mac;
+	dev->change_mtu = uml_net_change_mtu;
+	dev->ethtool_ops = &uml_net_ethtool_ops;
+	dev->watchdog_timeo = (HZ >> 1);
+	dev->irq = UM_ETH_IRQ;
 
-	return 0;
+	rtnl_lock();
+	err = register_netdevice(dev);
+	rtnl_unlock();
+	if (err)
+		goto out_undo_user_init;
+
+	spin_lock(&devices_lock);
+	list_add(&device->list, &devices);
+	spin_unlock(&devices_lock);
+
+	return;
+
+out_undo_user_init:
+	if (transport->user->init != NULL)
+		(*transport->user->remove)(&lp->user);
+out_unregister:
+	platform_device_unregister(&device->pdev);
+out_free_netdev:
+	free_netdev(dev);
+out_free_device: ;
+	kfree(device);
 }
 
 static struct uml_net *find_device(int n)
Index: linux-2.6.18-mm/arch/um/drivers/pcap_user.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/drivers/pcap_user.c	2007-02-20 16:12:28.000000000 -0500
+++ linux-2.6.18-mm/arch/um/drivers/pcap_user.c	2007-02-22 13:47:04.000000000 -0500
@@ -18,7 +18,7 @@
 
 #define PCAP_FD(p) (*(int *)(p))
 
-static void pcap_user_init(void *data, void *dev)
+static int pcap_user_init(void *data, void *dev)
 {
 	struct pcap_data *pri = data;
 	pcap_t *p;
@@ -28,11 +28,12 @@ static void pcap_user_init(void *data, v
 	if(p == NULL){
 		printk("pcap_user_init : pcap_open_live failed - '%s'\n", 
 		       errors);
-		return;
+		return -EINVAL;
 	}
 
 	pri->dev = dev;
 	pri->pcap = p;
+	return 0;
 }
 
 static int pcap_open(void *data)
@@ -42,19 +43,19 @@ static int pcap_open(void *data)
 	int err;
 
 	if(pri->pcap == NULL)
-		return(-ENODEV);
+		return -ENODEV;
 
 	if(pri->filter != NULL){
 		err = dev_netmask(pri->dev, &netmask);
 		if(err < 0){
 			printk("pcap_open : dev_netmask failed\n");
-			return(-EIO);
+			return -EIO;
 		}
 
 		pri->compiled = um_kmalloc(sizeof(struct bpf_program));
 		if(pri->compiled == NULL){
 			printk("pcap_open : kmalloc failed\n");
-			return(-ENOMEM);
+			return -ENOMEM;
 		}
 		
 		err = pcap_compile(pri->pcap, 
@@ -63,18 +64,18 @@ static int pcap_open(void *data)
 		if(err < 0){
 			printk("pcap_open : pcap_compile failed - '%s'\n", 
 			       pcap_geterr(pri->pcap));
-			return(-EIO);
+			return -EIO;
 		}
 
 		err = pcap_setfilter(pri->pcap, pri->compiled);
 		if(err < 0){
 			printk("pcap_open : pcap_setfilter failed - '%s'\n", 
 			       pcap_geterr(pri->pcap));
-			return(-EIO);
+			return -EIO;
 		}
 	}
 	
-	return(PCAP_FD(pri->pcap));
+	return PCAP_FD(pri->pcap);
 }
 
 static void pcap_remove(void *data)
@@ -114,11 +115,11 @@ int pcap_user_read(int fd, void *buffer,
 	n = pcap_dispatch(pri->pcap, 1, handler, (u_char *) &hdata);
 	if(n < 0){
 		printk("pcap_dispatch failed - %s\n", pcap_geterr(pri->pcap));
-		return(-EIO);
+		return -EIO;
 	}
 	else if(n == 0) 
-		return(0);
-	return(hdata.len);
+		return 0;
+	return hdata.len;
 }
 
 const struct net_user_info pcap_user_info = {
@@ -131,14 +132,3 @@ const struct net_user_info pcap_user_inf
 	.delete_address = NULL,
 	.max_packet	= MAX_PACKET - ETH_HEADER_OTHER
 };
-
-/*
- * Overrides for Emacs so that we follow Linus's tabbing style.
- * Emacs will notice this stuff at the end of the file and automatically
- * adjust the settings for this buffer only.  This must remain at the end
- * of the file.
- * ---------------------------------------------------------------------------
- * Local variables:
- * c-file-style: "linux"
- * End:
- */
Index: linux-2.6.18-mm/arch/um/drivers/slip_user.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/drivers/slip_user.c	2007-02-20 16:12:28.000000000 -0500
+++ linux-2.6.18-mm/arch/um/drivers/slip_user.c	2007-02-22 13:49:28.000000000 -0500
@@ -17,11 +17,12 @@
 #include "os.h"
 #include "um_malloc.h"
 
-void slip_user_init(void *data, void *dev)
+static int slip_user_init(void *data, void *dev)
 {
 	struct slip_data *pri = data;
 
 	pri->dev = dev;
+	return 0;
 }
 
 static int set_up_tty(int fd)
Index: linux-2.6.18-mm/arch/um/drivers/slirp_user.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/drivers/slirp_user.c	2007-02-20 16:12:28.000000000 -0500
+++ linux-2.6.18-mm/arch/um/drivers/slirp_user.c	2007-02-22 13:50:28.000000000 -0500
@@ -15,11 +15,12 @@
 #include "slip_common.h"
 #include "os.h"
 
-void slirp_user_init(void *data, void *dev)
+static int slirp_user_init(void *data, void *dev)
 {
 	struct slirp_data *pri = data;
 
 	pri->dev = dev;
+	return 0;
 }
 
 struct slirp_pre_exec_data {
Index: linux-2.6.18-mm/arch/um/include/net_user.h
===================================================================
--- linux-2.6.18-mm.orig/arch/um/include/net_user.h	2006-12-29 12:20:14.000000000 -0500
+++ linux-2.6.18-mm/arch/um/include/net_user.h	2007-02-22 13:51:11.000000000 -0500
@@ -14,7 +14,7 @@
 #define UML_NET_VERSION (4)
 
 struct net_user_info {
-	void (*init)(void *, void *);
+	int (*init)(void *, void *);
 	int (*open)(void *);
 	void (*close)(int, void *);
 	void (*remove)(void *);
Index: linux-2.6.18-mm/arch/um/os-Linux/drivers/ethertap_user.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/os-Linux/drivers/ethertap_user.c	2007-02-20 16:12:28.000000000 -0500
+++ linux-2.6.18-mm/arch/um/os-Linux/drivers/ethertap_user.c	2007-02-22 13:56:37.000000000 -0500
@@ -24,11 +24,12 @@
 
 #define MAX_PACKET ETH_MAX_PACKET
 
-void etap_user_init(void *data, void *dev)
+static int etap_user_init(void *data, void *dev)
 {
 	struct ethertap_data *pri = data;
 
 	pri->dev = dev;
+	return 0;
 }
 
 struct addr_change {
@@ -121,7 +122,7 @@ static int etap_tramp(char *dev, char *g
 	n = os_read_file(control_me, &c, sizeof(c));
 	if(n != sizeof(c)){
 		printk("etap_tramp : read of status failed, err = %d\n", -n);
-		return(-EINVAL);
+		return -EINVAL;
 	}
 	if(c != 1){
 		printk("etap_tramp : uml_net failed\n");
@@ -132,7 +133,7 @@ static int etap_tramp(char *dev, char *g
 		else if(!WIFEXITED(status) || (WEXITSTATUS(status) != 1))
 			printk("uml_net didn't exit with status 1\n");
 	}
-	return(err);
+	return err;
 }
 
 static int etap_open(void *data)
@@ -142,18 +143,19 @@ static int etap_open(void *data)
 	int data_fds[2], control_fds[2], err, output_len;
 
 	err = tap_open_common(pri->dev, pri->gate_addr);
-	if(err) return(err);
+	if(err)
+		return err;
 
 	err = os_pipe(data_fds, 0, 0);
 	if(err < 0){
 		printk("data os_pipe failed - err = %d\n", -err);
-		return(err);
+		return err;
 	}
 
 	err = os_pipe(control_fds, 1, 0);
 	if(err < 0){
 		printk("control os_pipe failed - err = %d\n", -err);
-		return(err);
+		return err;
 	}
 	
 	err = etap_tramp(pri->dev_name, pri->gate_addr, control_fds[0], 
@@ -171,13 +173,13 @@ static int etap_open(void *data)
 
 	if(err < 0){
 		printk("etap_tramp failed - err = %d\n", -err);
-		return(err);
+		return err;
 	}
 
 	pri->data_fd = data_fds[0];
 	pri->control_fd = control_fds[0];
 	iter_addresses(pri->dev, etap_open_addr, &pri->control_fd);
-	return(data_fds[0]);
+	return data_fds[0];
 }
 
 static void etap_close(int fd, void *data)
@@ -195,7 +197,7 @@ static void etap_close(int fd, void *dat
 
 static int etap_set_mtu(int mtu, void *data)
 {
-	return(mtu);
+	return mtu;
 }
 
 static void etap_add_addr(unsigned char *addr, unsigned char *netmask,
@@ -204,7 +206,8 @@ static void etap_add_addr(unsigned char 
 	struct ethertap_data *pri = data;
 
 	tap_check_ips(pri->gate_addr, addr);
-	if(pri->control_fd == -1) return;
+	if(pri->control_fd == -1)
+		return;
 	etap_open_addr(addr, netmask, &pri->control_fd);
 }
 
@@ -213,7 +216,8 @@ static void etap_del_addr(unsigned char 
 {
 	struct ethertap_data *pri = data;
 
-	if(pri->control_fd == -1) return;
+	if(pri->control_fd == -1)
+		return;
 	etap_close_addr(addr, netmask, &pri->control_fd);
 }
 
@@ -227,14 +231,3 @@ const struct net_user_info ethertap_user
 	.delete_address = etap_del_addr,
 	.max_packet	= MAX_PACKET - ETH_HEADER_ETHERTAP
 };
-
-/*
- * Overrides for Emacs so that we follow Linus's tabbing style.
- * Emacs will notice this stuff at the end of the file and automatically
- * adjust the settings for this buffer only.  This must remain at the end
- * of the file.
- * ---------------------------------------------------------------------------
- * Local variables:
- * c-file-style: "linux"
- * End:
- */
Index: linux-2.6.18-mm/arch/um/os-Linux/drivers/tuntap_user.c
===================================================================
--- linux-2.6.18-mm.orig/arch/um/os-Linux/drivers/tuntap_user.c	2007-02-20 16:12:28.000000000 -0500
+++ linux-2.6.18-mm/arch/um/os-Linux/drivers/tuntap_user.c	2007-02-22 13:55:28.000000000 -0500
@@ -24,11 +24,12 @@
 
 #define MAX_PACKET ETH_MAX_PACKET
 
-void tuntap_user_init(void *data, void *dev)
+static int tuntap_user_init(void *data, void *dev)
 {
 	struct tuntap_data *pri = data;
 
 	pri->dev = dev;
+	return 0;
 }
 
 static void tuntap_add_addr(unsigned char *addr, unsigned char *netmask,
@@ -37,7 +38,8 @@ static void tuntap_add_addr(unsigned cha
 	struct tuntap_data *pri = data;
 
 	tap_check_ips(pri->gate_addr, addr);
-	if((pri->fd == -1) || pri->fixed_config) return;
+	if((pri->fd == -1) || pri->fixed_config)
+		return;
 	open_addr(addr, netmask, pri->dev_name);
 }
 
@@ -46,7 +48,8 @@ static void tuntap_del_addr(unsigned cha
 {
 	struct tuntap_data *pri = data;
 
-	if((pri->fd == -1) || pri->fixed_config) return;
+	if((pri->fd == -1) || pri->fixed_config)
+		return;
 	close_addr(addr, netmask, pri->dev_name);
 }
 
@@ -83,7 +86,8 @@ static int tuntap_open_tramp(char *gate,
 
 	pid = run_helper(tuntap_pre_exec, &data, argv, NULL);
 
-	if(pid < 0) return(-pid);
+	if(pid < 0)
+		return -pid;
 
 	os_close_file(remote);
 
@@ -114,16 +118,16 @@ static int tuntap_open_tramp(char *gate,
 	cmsg = CMSG_FIRSTHDR(&msg);
 	if(cmsg == NULL){
 		printk("tuntap_open_tramp : didn't receive a message\n");
-		return(-EINVAL);
+		return -EINVAL;
 	}
 	if((cmsg->cmsg_level != SOL_SOCKET) || 
 	   (cmsg->cmsg_type != SCM_RIGHTS)){
 		printk("tuntap_open_tramp : didn't receive a descriptor\n");
-		return(-EINVAL);
+		return -EINVAL;
 	}
 	*fd_out = ((int *) CMSG_DATA(cmsg))[0];
 	os_set_exec_close(*fd_out, 1);
-	return(0);
+	return 0;
 }
 
 static int tuntap_open(void *data)
@@ -135,7 +139,7 @@ static int tuntap_open(void *data)
 
 	err = tap_open_common(pri->dev, pri->gate_addr);
 	if(err < 0)
-		return(err);
+		return err;
 
 	if(pri->fixed_config){
 		pri->fd = os_open_file("/dev/net/tun",
@@ -143,7 +147,7 @@ static int tuntap_open(void *data)
 		if(pri->fd < 0){
 			printk("Failed to open /dev/net/tun, err = %d\n",
 			       -pri->fd);
-			return(pri->fd);
+			return pri->fd;
 		}
 		memset(&ifr, 0, sizeof(ifr));
 		ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
@@ -160,7 +164,7 @@ static int tuntap_open(void *data)
 		if(err < 0){
 			printk("tuntap_open : os_pipe failed - err = %d\n",
 			       -err);
-			return(err);
+			return err;
 		}
 
 		buffer = get_output_buffer(&len);
@@ -175,7 +179,7 @@ static int tuntap_open(void *data)
 			printk("%s", output);
 			free_output_buffer(buffer);
 			printk("tuntap_open_tramp failed - err = %d\n", -err);
-			return(err);
+			return err;
 		}
 
 		pri->dev_name = uml_strdup(buffer);
@@ -187,7 +191,7 @@ static int tuntap_open(void *data)
 		iter_addresses(pri->dev, open_addr, pri->dev_name);
 	}
 
-	return(pri->fd);
+	return pri->fd;
 }
 
 static void tuntap_close(int fd, void *data)
@@ -202,7 +206,7 @@ static void tuntap_close(int fd, void *d
 
 static int tuntap_set_mtu(int mtu, void *data)
 {
-	return(mtu);
+	return mtu;
 }
 
 const struct net_user_info tuntap_user_info = {
@@ -215,14 +219,3 @@ const struct net_user_info tuntap_user_i
 	.delete_address = tuntap_del_addr,
 	.max_packet	= MAX_PACKET
 };
-
-/*
- * Overrides for Emacs so that we follow Linus's tabbing style.
- * Emacs will notice this stuff at the end of the file and automatically
- * adjust the settings for this buffer only.  This must remain at the end
- * of the file.
- * ---------------------------------------------------------------------------
- * Local variables:
- * c-file-style: "linux"
- * End:
- */

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] When opening /tmp/uml.ctl fails, what do we get?

[Blaisorblade]

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

On Thursday 22 February 2007 20:59, Jeff Dike wrote:
> On Thu, Feb 22, 2007 at 12:54:04AM +0100, Blaisorblade wrote:
> > I started uml_daemon as root by mistake, so /tmp/uml.ctl even if
> > /dev/net/tun was world-readable. Ok, I did it. This is the result (after
> > trying many times to do 'ifconfig eth0 up' inside UML):
>
> I don't see your irq stuff here when I try that, but there were
> multiple problems, which I hope are fixed by the patch below:
> 	style violations - this made the patch much bigger, and needs
> to be split out

Indeed, sigh. However I'm now convinced that if this just causes delays in 
patch merging without review benefit, it's not a good thing. It should be 
easy to split this away, and I have the patch about errno and am going to 
merge it (I hope to give a flush to my patch queue tomorrow).

However you still add new ones:

+       if(platform_device_register(&device->pdev))
+               goto out_free_netdev;

> 	the user init routines now return success or failure

Good.

> 	some major cleaning of the failure paths through eth_configure
> 		device was never freed
> 		sysfs_unregister was never called, adding the same
> interface again successfully would result in nasty error messages
> 		device stuck in the devices list regardless, so adding
> the same interface again would fail
> 		the return from platform_device_register was not
> checked

> 		I moved the register_netdev call until almost the end
> of the function

Have you checked if other calls require that register_netdev was already 
called?

> Let me know what you think about this.
>
> And about your thought of merging the _init into _open, my excuse for
> keeping them separate deserves some scrutiny.  Every transport besides
> the switch does all of its work in _open, and the _init routine just
> initializes data.  So, I would say that if a transport has separate
> plug-it-in and bring-it-up operations, maybe they should be separated
> more than they are now.  If the switch is the odd one, maybe we should
> merge _init into _open.

Hmm, I'm really not sure about what to do about this. However I'd leave that 
for a future cleanup and just add a comment about this.
And the reason for which init now returns a value is that pcap_init _can_ 
fail, so the switch transport is not the only odd one.

Another note:
+       /*
+        * This overallocates by sizeof(int) since the last field in
+        * uml_net_private is used to access the transport-specific
+        * data, so that user field is the first 4 bytes of the
+        * transport data.
+        */
+       size = transport->private_size + sizeof(struct uml_net_private);

There is a simple and standard solution in Linux style:

struct uml_net_private {
-        int user[1];
+        char user[0];
};

With fgrep [0] include/linux/* I found quite a few examples (like in 
include/linux/ethtool.h) - even if it is not fully ISO C conforming it's gcc 
conforming.

A proof-of-concept (i.e. untested and applying on vanilla) patch for this is 
attached, with name remember-zero-array-net.diff.

Another note: before of this patch, you should please apply the attached two 
ones.

*) The first (net-mac-check-cleanup.diff) checks the validity of assigned MAC 
address, but to print a meaningful error message requires adding a local 
buffer.
*) The second (net_kern-eth_configure...) allows avoiding this local buffer by 
moving code around.

I think I'm excessively paranoid about these two patches and about not yet 
merging them, so please give a look and merge them.
-- 
Inform me of my mistakes, so I can add them to my list!
Paolo Giarrusso, aka Blaisorblade
http://www.user-mode-linux.org/~blaisorblade

[-- Attachment #2: net-mac-check-cleanup.diff --]
[-- Type: text/x-diff, Size: 3881 bytes --]

Subject: uml: improve unicast MAC checking and enforce use of private MACs

Improve checking and diagnostics for broadcast and multicast Ethernet MAC
addresses, and distinguish between those cases in output; also make sure the
device is assigned a MAC address valid only locally to avoid collisions.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
Index: linux-2.6.git/arch/um/drivers/net_kern.c
===================================================================
--- linux-2.6.git.orig/arch/um/drivers/net_kern.c
+++ linux-2.6.git/arch/um/drivers/net_kern.c
@@ -284,7 +284,7 @@ void uml_net_user_timer_expire(unsigned 
 #endif
 }
 
-static void setup_etheraddr(char *str, unsigned char *addr)
+static void setup_etheraddr(char *str, unsigned char *addr, char *name)
 {
 	char *end;
 	int i;
@@ -303,15 +303,32 @@ static void setup_etheraddr(char *str, u
 		}
 		str = end + 1;
 	}
-	if(addr[0] & 1){
+	if (is_multicast_ether_addr(addr)) {
 		printk(KERN_ERR
-		       "Attempt to assign a broadcast ethernet address to a "
+		       "Attempt to assign a multicast ethernet address to a "
 		       "device disallowed\n");
 		goto random;
 	}
+	if (!is_valid_ether_addr(addr)) {
+		printk(KERN_ERR
+		       "Attempt to assign an invalid ethernet address to a "
+		       "device disallowed\n");
+		goto random;
+	}
+	if (!is_local_ether_addr(addr)) {
+		printk(KERN_WARNING
+		       "Warning: attempt to assign a globally valid ethernet address to a "
+		       "device\n");
+		printk(KERN_WARNING "You should better enable the 2nd rightmost bit "
+		      "in the first byte of the MAC, i.e. "
+		      "%02x:%02x:%02x:%02x:%02x:%02x\n",
+		      addr[0] | 0x02, addr[1], addr[2], addr[3], addr[4], addr[5]);
+	}
 	return;
 
 random:
+	printk(KERN_INFO
+	       "Choosing a random ethernet address for device %s\n", name);
 	random_ether_addr(addr);
 }
 
@@ -332,6 +349,7 @@ static int eth_configure(int n, void *in
 	struct net_device *dev;
 	struct uml_net_private *lp;
 	int save, err, size;
+	char name[sizeof(dev->name)];
 
 	size = transport->private_size + sizeof(struct uml_net_private) +
 		sizeof(((struct uml_net_private *) 0)->user);
@@ -349,7 +367,13 @@ static int eth_configure(int n, void *in
 	list_add(&device->list, &devices);
 	spin_unlock(&devices_lock);
 
-	setup_etheraddr(mac, device->mac);
+	/* If this name ends up conflicting with an existing registered
+	 * netdevice, that is OK, register_netdev{,ice}() will notice this
+	 * and fail.
+	 */
+	snprintf(name, sizeof(name), "eth%d", n);
+
+	setup_etheraddr(mac, device->mac, name);
 
 	printk(KERN_INFO "Netdevice %d ", n);
 	printk("(%02x:%02x:%02x:%02x:%02x:%02x) ",
@@ -379,11 +403,7 @@ static int eth_configure(int n, void *in
 	platform_device_register(&device->pdev);
 	SET_NETDEV_DEV(dev,&device->pdev.dev);
 
-	/* If this name ends up conflicting with an existing registered
-	 * netdevice, that is OK, register_netdev{,ice}() will notice this
-	 * and fail.
-	 */
-	snprintf(dev->name, sizeof(dev->name), "eth%d", n);
+	strcpy(dev->name, name);
 	device->dev = dev;
 
 	(*transport->kern->init)(dev, init);
Index: linux-2.6.git/include/linux/etherdevice.h
===================================================================
--- linux-2.6.git.orig/include/linux/etherdevice.h
+++ linux-2.6.git/include/linux/etherdevice.h
@@ -71,6 +71,18 @@ static inline int is_multicast_ether_add
 }
 
 /**
+ * is_local_ether_addr - Determine if the Ethernet address is locally-assigned
+ * one (IEEE 802).
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Return true if the address is a local address.
+ */
+static inline int is_local_ether_addr(const u8 *addr)
+{
+	return (0x02 & addr[0]);
+}
+
+/**
  * is_broadcast_ether_addr - Determine if the Ethernet address is broadcast
  * @addr: Pointer to a six-byte array containing the Ethernet address
  *

[-- Attachment #3: net_kern-eth_configure-avoid-tmp-buf.diff --]
[-- Type: text/x-diff, Size: 2691 bytes --]

Subject: uml net: revert introduction of private buffer

Avoid using the temporary buffer introduced by previous patch to hold the device name.
Btw, avoid leaking device on an error path. Other error paths may need cleanup.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
Index: linux-2.6.git/arch/um/drivers/net_kern.c
===================================================================
--- linux-2.6.git.orig/arch/um/drivers/net_kern.c
+++ linux-2.6.git/arch/um/drivers/net_kern.c
@@ -349,15 +349,20 @@ static int eth_configure(int n, void *in
 	struct net_device *dev;
 	struct uml_net_private *lp;
 	int save, err, size;
-	char name[sizeof(dev->name)];
 
 	size = transport->private_size + sizeof(struct uml_net_private) +
 		sizeof(((struct uml_net_private *) 0)->user);
 
 	device = kzalloc(sizeof(*device), GFP_KERNEL);
 	if (device == NULL) {
-		printk(KERN_ERR "eth_configure failed to allocate uml_net\n");
-		return(1);
+		printk(KERN_ERR "eth_configure failed to allocate struct uml_net\n");
+		return 1;
+	}
+
+	dev = alloc_etherdev(size);
+	if (dev == NULL) {
+		printk(KERN_ERR "eth_configure: failed to allocate struct net_device for eth%d\n", n);
+		return 1;
 	}
 
 	INIT_LIST_HEAD(&device->list);
@@ -371,9 +376,9 @@ static int eth_configure(int n, void *in
 	 * netdevice, that is OK, register_netdev{,ice}() will notice this
 	 * and fail.
 	 */
-	snprintf(name, sizeof(name), "eth%d", n);
+	snprintf(dev->name, sizeof(dev->name), "eth%d", n);
 
-	setup_etheraddr(mac, device->mac, name);
+	setup_etheraddr(mac, device->mac, dev->name);
 
 	printk(KERN_INFO "Netdevice %d ", n);
 	printk("(%02x:%02x:%02x:%02x:%02x:%02x) ",
@@ -381,11 +386,6 @@ static int eth_configure(int n, void *in
 	       device->mac[2], device->mac[3],
 	       device->mac[4], device->mac[5]);
 	printk(": ");
-	dev = alloc_etherdev(size);
-	if (dev == NULL) {
-		printk(KERN_ERR "eth_configure: failed to allocate device\n");
-		return 1;
-	}
 
 	lp = dev->priv;
 	/* This points to the transport private data. It's still clear, but we
@@ -403,7 +403,6 @@ static int eth_configure(int n, void *in
 	platform_device_register(&device->pdev);
 	SET_NETDEV_DEV(dev,&device->pdev.dev);
 
-	strcpy(dev->name, name);
 	device->dev = dev;
 
 	(*transport->kern->init)(dev, init);
@@ -426,8 +425,12 @@ static int eth_configure(int n, void *in
 	rtnl_unlock();
 	if (err) {
 		device->dev = NULL;
-		/* XXX: should we call ->remove() here? */
+		/* XXX: should we call ->remove() here?
+		 * IMHO no, kern->remove does
+		 * not exist because allocations are done here only, and
+		 * user->init has not been called yet.*/
 		free_netdev(dev);
+		kfree(device);
 		return 1;
 	}
 

[-- Attachment #4: remember-zero-array-net.diff --]
[-- Type: text/x-diff, Size: 2392 bytes --]

uml: Show how user can be converted to a zero-array.

To look at users I did:
$ find arch/um/ include/asm-um -name '*.[ch]'|xargs grep -r 'net_kern\.h' -l|xargs grep '\<user\>'

Most users just cast user to the appropriate pointer, the remaining ones are
fixed here. In net_kern.c, I'm almost sure that save trick is not needed
anymore, but I've not verified it.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
Index: linux-2.6.git/arch/um/include/net_kern.h
===================================================================
--- linux-2.6.git.orig/arch/um/include/net_kern.h
+++ linux-2.6.git/arch/um/include/net_kern.h
@@ -40,7 +40,7 @@ struct uml_net_private {
 	void (*add_address)(unsigned char *, unsigned char *, void *);
 	void (*delete_address)(unsigned char *, unsigned char *, void *);
 	int (*set_mtu)(int mtu, void *);
-	int user[1];
+	char user[0];
 };
 
 struct net_kern_info {
Index: linux-2.6.git/arch/um/drivers/net_kern.c
===================================================================
--- linux-2.6.git.orig/arch/um/drivers/net_kern.c
+++ linux-2.6.git/arch/um/drivers/net_kern.c
@@ -331,10 +331,9 @@ static int eth_configure(int n, void *in
 	struct uml_net *device;
 	struct net_device *dev;
 	struct uml_net_private *lp;
-	int save, err, size;
+	int err, size;
 
-	size = transport->private_size + sizeof(struct uml_net_private) +
-		sizeof(((struct uml_net_private *) 0)->user);
+	size = transport->private_size + sizeof(struct uml_net_private);
 
 	device = kzalloc(sizeof(*device), GFP_KERNEL);
 	if (device == NULL) {
@@ -411,12 +410,6 @@ static int eth_configure(int n, void *in
 		return 1;
 	}
 
-	/* lp.user is the first four bytes of the transport data, which
-	 * has already been initialized.  This structure assignment will
-	 * overwrite that, so we make sure that .user gets overwritten with
-	 * what it already has.
-	 */
-	save = lp->user[0];
 	*lp = ((struct uml_net_private)
 		{ .list  		= LIST_HEAD_INIT(lp->list),
 		  .dev 			= dev,
@@ -430,8 +423,7 @@ static int eth_configure(int n, void *in
 		  .write 		= transport->kern->write,
 		  .add_address 		= transport->user->add_address,
 		  .delete_address  	= transport->user->delete_address,
-		  .set_mtu 		= transport->user->set_mtu,
-		  .user  		= { save } });
+		  .set_mtu 		= transport->user->set_mtu });
 
 	init_timer(&lp->tl);
 	spin_lock_init(&lp->lock);

[-- Attachment #5: Type: text/plain, Size: 345 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #6: Type: text/plain, Size: 194 bytes --]

_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] When opening /tmp/uml.ctl fails, what do we get?

[Jeff Dike]

On Sun, Mar 04, 2007 at 08:13:39PM +0100, Blaisorblade wrote:
> Have you checked if other calls require that register_netdev was already 
> called?

Just did, nothing else depends on it.

> Hmm, I'm really not sure about what to do about this. However I'd leave that 
> for a future cleanup and just add a comment about this.
> And the reason for which init now returns a value is that pcap_init _can_ 
> fail, so the switch transport is not the only odd one.

Yeah, that slipped my mind.

> struct uml_net_private {
> -        int user[1];
> +        char user[0];
> };

Neat, that used to be illegal.

> Another note: before of this patch, you should please apply the attached two 
> ones.
> 
> *) The first (net-mac-check-cleanup.diff) checks the validity of assigned MAC 
> address, but to print a meaningful error message requires adding a local 
> buffer.
> *) The second (net_kern-eth_configure...) allows avoiding this local buffer by 
> moving code around.
> 
> I think I'm excessively paranoid about these two patches and about not yet 
> merging them, so please give a look and merge them.

I just dropped them in - they look OK at a first glance.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] When opening /tmp/uml.ctl fails, what do we get?

[Blaisorblade]

On Monday 05 March 2007 16:51, Jeff Dike wrote:
> On Sun, Mar 04, 2007 at 08:13:39PM +0100, Blaisorblade wrote:
> > Have you checked if other calls require that register_netdev was already
> > called?
>
> Just did, nothing else depends on it.
Fine.
> > Hmm, I'm really not sure about what to do about this. However I'd leave
> > that for a future cleanup and just add a comment about this.
> > And the reason for which init now returns a value is that pcap_init _can_
> > fail, so the switch transport is not the only odd one.
>
> Yeah, that slipped my mind.
>
> > struct uml_net_private {
> > -        int user[1];
> > +        char user[0];
> > };
>
> Neat, that used to be illegal.

If you use -pedantic (-ansi -std=c89 doesn't suffice) you'll learn that ISO C 
prohibits that; however, ethtool.h is quite a good example I guess, so GCC 
3.2 supports that; and the code is anyway simpler that way.

-- 
Inform me of my mistakes, so I can add them to my list!
Paolo Giarrusso, aka Blaisorblade
http://www.user-mode-linux.org/~blaisorblade
Chiacchiera con i tuoi amici in tempo reale! 
 http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com 


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel