Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Klaus Dittrich]

What speaks against allocating the buffer in kset_hotplug_ops.hotplug()
and adding the env variables later in a more consistent way by calls to
add_hotplug_env_var()?

-- 
Klaus

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Greg KH]

On Wed, Oct 27, 2004 at 10:29:25AM -0400, Andrew Duggan wrote:
> Hi,
> 
> In the lastest code I've seen 2.6.10 rc1 bk6 for lib/kobject_uevent.c
> the incrementing of the sequence number is held off until the call to
> hotplug_ops->hotplug ().  That function can consume addtional buffer
> and use addtional pointers in the envp array.  The problem is that the
> new value for i and the new logical end of buffer (held in scratch) do
> not get updated.  When the sequnce number is then processed the first
> item generated by hotplug_ops->hotplug () gets overwriten, and can
> potentially corrupt any env vars created by hotplug_ops->hotplug.

Ah, good catch.  Sorry about adding that bug.

> Here is a patch to handle that.  An element in envp is reserved for
> the seq num and a different buffer is sent to hotplug_ops->hotplug
> altogether. Using a different buffer there is OK since send_uevent
> wasn't seeing the part of the buffer filled in by hotplug_ops->hotplug
> anyway, and the older call_usermodehelper gets passed envp and not
> buffer. 

Why not just use the same buffer?  We should be able to do that.

> I used the same size for the buffer_pt2 as buffer for simplicity,
> since I didn't know if saving the memory was THAT important. If need
> be the kmalloc for buffer_pt2 could be held off until just before the
> call to hotplug_ops->hotplug so that it could be allocated only
> (BUFFERSIZE - (scratch - buffer)).  

I'd prefer we use the same buffer.  Care to respin your patch?

thanks,

greg k-h

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Andrew]

Greg KH wrote:
> 
> 
> Why not just use the same buffer?  We should be able to do that.
> 
> 
> I'd prefer we use the same buffer.  Care to respin your patch?
> 

Sure, I can only see two ways of achieving that however.
1) Change the API of kset_hotplug_ops.hotplug() to return the amount
    of consumed buffer (and possibly an updated value for i (num_envp)
    and then changing every real function that implements that interface
    or
2) Spin through the envp[] starting at i to NUM_ENVP looking for a NULL
    pointer dropping back 1 (last_used) then do a
    scratch += strlen(envp[last_used]) + 1

I don't know if you would rather keep the kset_hotplug_ops.hotplug() API
unchanged and have a "find the NULL" or vice-versa? -- or are both
too ugly? Ultimately the first option would generate the "cleanest"
solution, but I would be wary to change an API (especially one I didn't
create myself)

I can also see variation of 2), where buffer is prefilled with something
like '0xff' then the find the null would run in reverse from
buffer+BUFFER_SIZE-1.  Either way with 2), the reservation in envp[] for
the seq_num would have to stay, but its locaton in buffer would be at the
end.  As a bonus of course then the call to send_uevent could see the
entire buffer.

I'm looking now for how many changes would be required to do option 1.

Andrew



> thanks,
> 
> greg k-h
> 
> 

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Greg KH]

On Wed, Oct 27, 2004 at 12:31:52PM -0400, Andrew wrote:
> 
> Greg KH wrote:
> >
> >
> >Why not just use the same buffer?  We should be able to do that.
> >
> >
> >I'd prefer we use the same buffer.  Care to respin your patch?
> >
> 
> Sure, I can only see two ways of achieving that however.
> 1) Change the API of kset_hotplug_ops.hotplug() to return the amount
>    of consumed buffer (and possibly an updated value for i (num_envp)
>    and then changing every real function that implements that interface
>    or
> 2) Spin through the envp[] starting at i to NUM_ENVP looking for a NULL
>    pointer dropping back 1 (last_used) then do a
>    scratch += strlen(envp[last_used]) + 1

Ick, ok, let's stick with 2 buffers.  How about the patch below?  It's a
bit smaller than yours.

But there might still be a problem.  With this change, the sequence
number is not sent out the kevent message.  Kay, do you think this is an
issue?  I don't think we can get netlink messages out of order, right?

I'll hold off on applying this patch until we figure this out...

thanks,

greg k-h


--- 1.5/lib/kobject_uevent.c	2004-10-22 17:42:27 -05:00
+++ edited/kobject_uevent.c	2004-10-29 15:05:32 -05:00
@@ -182,6 +182,7 @@
 	char *argv [3];
 	char **envp = NULL;
 	char *buffer = NULL;
+	char *seq_buff = NULL;
 	char *scratch;
 	int i = 0;
 	int retval;
@@ -229,6 +230,9 @@
 	buffer = kmalloc(BUFFER_SIZE, GFP_KERNEL);
 	if (!buffer)
 		goto exit;
+	seq_buff = kmalloc(BUFFER_SIZE, GFP_KERNEL);
+	if (!seq_buff)
+		goto exit;
 
 	if (hotplug_ops->name)
 		name = hotplug_ops->name(kset, kobj);
@@ -258,6 +262,9 @@
 	envp [i++] = scratch;
 	scratch += sprintf(scratch, "SUBSYSTEM=%s", name) + 1;
 
+	/* point to the buffer, but don't fill it until after the hotplug call */
+	envp [i++] = seq_buff;
+
 	if (hotplug_ops->hotplug) {
 		/* have the kset specific function add its stuff */
 		retval = hotplug_ops->hotplug (kset, kobj,
@@ -273,9 +280,7 @@
 	spin_lock(&sequence_lock);
 	seq = ++hotplug_seqnum;
 	spin_unlock(&sequence_lock);
-
-	envp [i++] = scratch;
-	scratch += sprintf(scratch, "SEQNUM=%lld", (long long)seq) + 1;
+	sprintf(seq_buff, "SEQNUM=%lld", (long long)seq);
 
 	pr_debug ("%s: %s %s seq=%lld %s %s %s %s %s\n",
 		  __FUNCTION__, argv[0], argv[1], (long long)seq,
@@ -293,6 +298,7 @@
 
 exit:
 	kfree(kobj_path);
+	kfree(seq_buff);
 	kfree(buffer);
 	kfree(envp);
 	return;

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Kay Sievers]

On Fri, Oct 29, 2004 at 03:13:15PM -0500, Greg KH wrote:
> On Wed, Oct 27, 2004 at 12:31:52PM -0400, Andrew wrote:
> > 
> > Greg KH wrote:
> > >
> > >
> > >Why not just use the same buffer?  We should be able to do that.
> > >
> > >
> > >I'd prefer we use the same buffer.  Care to respin your patch?
> > >
> > 
> > Sure, I can only see two ways of achieving that however.
> > 1) Change the API of kset_hotplug_ops.hotplug() to return the amount
> >    of consumed buffer (and possibly an updated value for i (num_envp)
> >    and then changing every real function that implements that interface
> >    or
> > 2) Spin through the envp[] starting at i to NUM_ENVP looking for a NULL
> >    pointer dropping back 1 (last_used) then do a
> >    scratch += strlen(envp[last_used]) + 1
> 
> Ick, ok, let's stick with 2 buffers.  How about the patch below?  It's a
> bit smaller than yours.
> 
> But there might still be a problem.  With this change, the sequence
> number is not sent out the kevent message.  Kay, do you think this is an
> issue?  I don't think we can get netlink messages out of order, right?

Right, especially not the events with the same DEVPATH, like "remove"
beating an "add". But I'm not sure if the number isn't useful. Whatever
we may do with the hotplug over netlink in the future, we will only have
/sbin/hotplug for the early boot and it may be nice to know, what events
we have already handled...

> I'll hold off on applying this patch until we figure this out...

How about just reserving 20 bytes for the number (u64 will never be
more than that), save the pointer to that field, and fill the number in
later?

Thanks,
Kay

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Greg KH]

On Fri, Oct 29, 2004 at 11:28:56PM +0200, Kay Sievers wrote:
> > But there might still be a problem.  With this change, the sequence
> > number is not sent out the kevent message.  Kay, do you think this is an
> > issue?  I don't think we can get netlink messages out of order, right?
> 
> Right, especially not the events with the same DEVPATH, like "remove"
> beating an "add". But I'm not sure if the number isn't useful. Whatever
> we may do with the hotplug over netlink in the future, we will only have
> /sbin/hotplug for the early boot and it may be nice to know, what events
> we have already handled...
> 
> > I'll hold off on applying this patch until we figure this out...
> 
> How about just reserving 20 bytes for the number (u64 will never be
> more than that), save the pointer to that field, and fill the number in
> later?

Ah, something like this instead?  I like it, it's even smaller than the
previous patch.  Compile tested only...

thanks,

greg k-h

--------

--- 1.5/lib/kobject_uevent.c	2004-10-22 17:42:27 -05:00
+++ edited/kobject_uevent.c	2004-10-29 18:07:50 -05:00
@@ -182,6 +182,7 @@
 	char *argv [3];
 	char **envp = NULL;
 	char *buffer = NULL;
+	char *seq_buff;
 	char *scratch;
 	int i = 0;
 	int retval;
@@ -258,6 +259,11 @@
 	envp [i++] = scratch;
 	scratch += sprintf(scratch, "SUBSYSTEM=%s", name) + 1;
 
+	/* reserve space for the sequence, 
+	 * put the real one in after the hotplug call */
+	envp[i++] = seq_buff = scratch;
+	scratch += sprintf(scratch, "SEQNUM=12345678901234567890") + 1;
+
 	if (hotplug_ops->hotplug) {
 		/* have the kset specific function add its stuff */
 		retval = hotplug_ops->hotplug (kset, kobj,
@@ -273,9 +279,7 @@
 	spin_lock(&sequence_lock);
 	seq = ++hotplug_seqnum;
 	spin_unlock(&sequence_lock);
-
-	envp [i++] = scratch;
-	scratch += sprintf(scratch, "SEQNUM=%lld", (long long)seq) + 1;
+	sprintf(seq_buff, "SEQNUM=%lld", (long long)seq);
 
 	pr_debug ("%s: %s %s seq=%lld %s %s %s %s %s\n",
 		  __FUNCTION__, argv[0], argv[1], (long long)seq,

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Kay Sievers]

On Fri, Oct 29, 2004 at 06:13:19PM -0500, Greg KH wrote:
> On Fri, Oct 29, 2004 at 11:28:56PM +0200, Kay Sievers wrote:
> > > But there might still be a problem.  With this change, the sequence
> > > number is not sent out the kevent message.  Kay, do you think this is an
> > > issue?  I don't think we can get netlink messages out of order, right?
> > 
> > Right, especially not the events with the same DEVPATH, like "remove"
> > beating an "add". But I'm not sure if the number isn't useful. Whatever
> > we may do with the hotplug over netlink in the future, we will only have
> > /sbin/hotplug for the early boot and it may be nice to know, what events
> > we have already handled...
> > 
> > > I'll hold off on applying this patch until we figure this out...
> > 
> > How about just reserving 20 bytes for the number (u64 will never be
> > more than that), save the pointer to that field, and fill the number in
> > later?
> 
> Ah, something like this instead?  I like it, it's even smaller than the
> previous patch.  Compile tested only...

I like that. How about the following. It will keep the buffer clean from
random chars, cause the kevent does not have the vector and relies on
the '\0' to separate the strings from each other.
I've tested it. The netlink-hotplug message looks like this:

recv(3, "remove@/class/input/mouse2\0ACTION=remove\0DEVPATH=/class/input/mouse2\0SUBSYSTEM=input\0SEQNUM=961                 \0", 1024, 0) = 113


> --------
> 
> --- 1.5/lib/kobject_uevent.c	2004-10-22 17:42:27 -05:00
> +++ edited/kobject_uevent.c	2004-10-29 18:07:50 -05:00
> @@ -182,6 +182,7 @@
>  	char *argv [3];
>  	char **envp = NULL;
>  	char *buffer = NULL;
> +	char *seq_buff;
>  	char *scratch;
>  	int i = 0;
>  	int retval;
> @@ -258,6 +259,11 @@
>  	envp [i++] = scratch;
>  	scratch += sprintf(scratch, "SUBSYSTEM=%s", name) + 1;
>  
> +	/* reserve space for the sequence, 
> +	 * put the real one in after the hotplug call */
> +	envp[i++] = seq_buff = scratch;
> +	scratch += sprintf(scratch, "SEQNUM=12345678901234567890") + 1;

+       scratch += strlen("SEQNUM=12345678901234567890") + 1;

> +
>  	if (hotplug_ops->hotplug) {
>  		/* have the kset specific function add its stuff */
>  		retval = hotplug_ops->hotplug (kset, kobj,
> @@ -273,9 +279,7 @@
>  	spin_lock(&sequence_lock);
>  	seq = ++hotplug_seqnum;
>  	spin_unlock(&sequence_lock);
> -
> -	envp [i++] = scratch;
> -	scratch += sprintf(scratch, "SEQNUM=%lld", (long long)seq) + 1;
> +	sprintf(seq_buff, "SEQNUM=%lld", (long long)seq);

+       sprintf(seq_buff, "SEQNUM=%-20lld", (long long)seq);

>  	pr_debug ("%s: %s %s seq=%lld %s %s %s %s %s\n",
>  		  __FUNCTION__, argv[0], argv[1], (long long)seq,
> 

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Kay Sievers]

On Sat, Oct 30, 2004 at 02:00:45AM +0200, Kay Sievers wrote:
> On Fri, Oct 29, 2004 at 06:13:19PM -0500, Greg KH wrote:
> > On Fri, Oct 29, 2004 at 11:28:56PM +0200, Kay Sievers wrote:
> > > > But there might still be a problem.  With this change, the sequence
> > > > number is not sent out the kevent message.  Kay, do you think this is an
> > > > issue?  I don't think we can get netlink messages out of order, right?
> > > 
> > > Right, especially not the events with the same DEVPATH, like "remove"
> > > beating an "add". But I'm not sure if the number isn't useful. Whatever
> > > we may do with the hotplug over netlink in the future, we will only have
> > > /sbin/hotplug for the early boot and it may be nice to know, what events
> > > we have already handled...
> > > 
> > > > I'll hold off on applying this patch until we figure this out...
> > > 
> > > How about just reserving 20 bytes for the number (u64 will never be
> > > more than that), save the pointer to that field, and fill the number in
> > > later?
> > 
> > Ah, something like this instead?  I like it, it's even smaller than the
> > previous patch.  Compile tested only...
> 
> I like that. How about the following. It will keep the buffer clean from
> random chars, cause the kevent does not have the vector and relies on
> the '\0' to separate the strings from each other.
> I've tested it. The netlink-hotplug message looks like this:
> 
> recv(3, "remove@/class/input/mouse2\0ACTION=remove\0DEVPATH=/class/input/mouse2\0SUBSYSTEM=input\0SEQNUM=961                 \0", 1024, 0) = 113

Hmm, these trailing spaces are just bad, sorry. I'll better pass the
envp array over to send_uevent() and clean up the keys while copying
the env values into the skb buffer. This will make the event payload
more safe too. So your first version looks better.

Thanks,
Kay

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Kay Sievers]

On Sat, Oct 30, 2004 at 02:25:23AM +0200, Kay Sievers wrote:
> On Sat, Oct 30, 2004 at 02:00:45AM +0200, Kay Sievers wrote:
> > On Fri, Oct 29, 2004 at 06:13:19PM -0500, Greg KH wrote:
> > > On Fri, Oct 29, 2004 at 11:28:56PM +0200, Kay Sievers wrote:
> > > > > But there might still be a problem.  With this change, the sequence
> > > > > number is not sent out the kevent message.  Kay, do you think this is an
> > > > > issue?  I don't think we can get netlink messages out of order, right?
> > > > 
> > > > Right, especially not the events with the same DEVPATH, like "remove"
> > > > beating an "add". But I'm not sure if the number isn't useful. Whatever
> > > > we may do with the hotplug over netlink in the future, we will only have
> > > > /sbin/hotplug for the early boot and it may be nice to know, what events
> > > > we have already handled...
> > > > 
> > > > > I'll hold off on applying this patch until we figure this out...
> > > > 
> > > > How about just reserving 20 bytes for the number (u64 will never be
> > > > more than that), save the pointer to that field, and fill the number in
> > > > later?
> > > 
> > > Ah, something like this instead?  I like it, it's even smaller than the
> > > previous patch.  Compile tested only...
> > 
> > I like that. How about the following. It will keep the buffer clean from
> > random chars, cause the kevent does not have the vector and relies on
> > the '\0' to separate the strings from each other.
> > I've tested it. The netlink-hotplug message looks like this:
> > 
> > recv(3, "remove@/class/input/mouse2\0ACTION=remove\0DEVPATH=/class/input/mouse2\0SUBSYSTEM=input\0SEQNUM=961                 \0", 1024, 0) = 113
> 
> Hmm, these trailing spaces are just bad, sorry. I'll better pass the
> envp array over to send_uevent() and clean up the keys while copying
> the env values into the skb buffer. This will make the event payload
> more safe too. So your first version looks better.

How about this? We copy over key by key into the skb buffer and the
netlink message can get the envp array without depending on a single
continuous buffer.

The netlink message looks nice like this now:

recv(3, "
  add@/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.0\0
  HOME=/\0
  PATH=/sbin:/bin:/usr/sbin:/usr/bin\0
  ACTION=add\0
  DEVPATH=/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.0\0
  SUBSYSTEM=usb\0
  SEQNUM=991\0
  DEVICE=/proc/bus/usb/003/008\0
  PRODUCT=46d/c03e/2000\0
  TYPE=0/0/0\0
  INTERFACE=3/1/2\0
", 1024, 0) = 268


Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>

===== lib/kobject_uevent.c 1.10 vs edited =====
--- 1.10/lib/kobject_uevent.c	2004-10-23 00:42:52 +02:00
+++ edited/lib/kobject_uevent.c	2004-10-30 03:53:18 +02:00
@@ -23,6 +23,9 @@
 #include <linux/kobject.h>
 #include <net/sock.h>
 
+#define BUFFER_SIZE	1024	/* buffer for the hotplug env */
+#define NUM_ENVP	32	/* number of env pointers */
+
 #if defined(CONFIG_KOBJECT_UEVENT) || defined(CONFIG_HOTPLUG)
 static char *action_to_string(enum kobject_action action)
 {
@@ -57,28 +60,40 @@ static struct sock *uevent_sock;
  * @buflen:
  * gfp_mask:
  */
-static int send_uevent(const char *signal, const char *obj, const void *buf,
-			int buflen, int gfp_mask)
+static int send_uevent(const char *signal, const char *obj,
+		       char **envp, int gfp_mask)
 {
 	struct sk_buff *skb;
 	char *pos;
-	int len;
+	int i;
+	int skblen, linelen;
 
 	if (!uevent_sock)
 		return -EIO;
 
-	len = strlen(signal) + 1;
-	len += strlen(obj) + 1;
-	len += buflen;
+	skblen = strlen(signal) + 1;
+	skblen += strlen(obj) + 1;
 
-	skb = alloc_skb(len, gfp_mask);
+	/* allocate buffer with maximum message size */
+	skb = alloc_skb(skblen + BUFFER_SIZE, gfp_mask);
 	if (!skb)
 		return -ENOMEM;
 
-	pos = skb_put(skb, len);
+	pos = skb_put(skb, skblen + BUFFER_SIZE);
 
 	pos += sprintf(pos, "%s@%s", signal, obj) + 1;
-	memcpy(pos, buf, buflen);
+
+	/* copy the environment key by key to our '\0' delimited buffer */
+	if (envp) {
+		for (i = 0; envp[i]; i++) {
+			linelen = strlcpy(pos, envp[i], BUFFER_SIZE) + 1;
+			skblen += linelen;
+			pos += linelen;
+		}
+	}
+
+	/* trim skb buffer to the actual used size */
+	skb_trim(skb, skblen);
 
 	return netlink_broadcast(uevent_sock, skb, 0, 1, gfp_mask);
 }
@@ -107,10 +122,10 @@ static int do_kobject_uevent(struct kobj
 		if (!attrpath)
 			goto exit;
 		sprintf(attrpath, "%s/%s", path, attr->name);
-		rc = send_uevent(signal, attrpath, NULL, 0, gfp_mask);
+		rc = send_uevent(signal, attrpath, NULL, gfp_mask);
 		kfree(attrpath);
 	} else {
-		rc = send_uevent(signal, path, NULL, 0, gfp_mask);
+		rc = send_uevent(signal, path, NULL, gfp_mask);
 	}
 
 exit:
@@ -169,8 +184,6 @@ static inline int send_uevent(const char
 u64 hotplug_seqnum;
 static spinlock_t sequence_lock = SPIN_LOCK_UNLOCKED;
 
-#define BUFFER_SIZE	1024	/* should be enough memory for the env */
-#define NUM_ENVP	32	/* number of env pointers */
 /**
  * kobject_hotplug - notify userspace by executing /sbin/hotplug
  *
@@ -182,6 +195,7 @@ void kobject_hotplug(struct kobject *kob
 	char *argv [3];
 	char **envp = NULL;
 	char *buffer = NULL;
+	char *seq_buff;
 	char *scratch;
 	int i = 0;
 	int retval;
@@ -258,6 +272,11 @@ void kobject_hotplug(struct kobject *kob
 	envp [i++] = scratch;
 	scratch += sprintf(scratch, "SUBSYSTEM=%s", name) + 1;
 
+	/* reserve space for the sequence, 
+	 * put the real one in after the hotplug call */
+	envp[i++] = seq_buff = scratch;
+	scratch += strlen("SEQNUM=18446744073709551616") + 1;
+
 	if (hotplug_ops->hotplug) {
 		/* have the kset specific function add its stuff */
 		retval = hotplug_ops->hotplug (kset, kobj,
@@ -273,15 +292,13 @@ void kobject_hotplug(struct kobject *kob
 	spin_lock(&sequence_lock);
 	seq = ++hotplug_seqnum;
 	spin_unlock(&sequence_lock);
-
-	envp [i++] = scratch;
-	scratch += sprintf(scratch, "SEQNUM=%lld", (long long)seq) + 1;
+	sprintf(seq_buff, "SEQNUM=%lld", (long long)seq);
 
 	pr_debug ("%s: %s %s seq=%lld %s %s %s %s %s\n",
 		  __FUNCTION__, argv[0], argv[1], (long long)seq,
 		  envp[0], envp[1], envp[2], envp[3], envp[4]);
 
-	send_uevent(action_string, kobj_path, buffer, scratch - buffer, GFP_KERNEL);
+	send_uevent(action_string, kobj_path, envp, GFP_KERNEL);
 
 	if (!hotplug_path[0])
 		goto exit;

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Andrew]

Kay Sievers wrote:
> On Sat, Oct 30, 2004 at 02:25:23AM +0200, Kay Sievers wrote:
> 
>>On Sat, Oct 30, 2004 at 02:00:45AM +0200, Kay Sievers wrote:
>>
>>>On Fri, Oct 29, 2004 at 06:13:19PM -0500, Greg KH wrote:
>>>
>>>>On Fri, Oct 29, 2004 at 11:28:56PM +0200, Kay Sievers wrote:
>>>>
>>>>>>But there might still be a problem.  With this change, the sequence
>>>>>>number is not sent out the kevent message.  Kay, do you think this is an
>>>>>>issue?  I don't think we can get netlink messages out of order, right?
>>>>>
>>>>>Right, especially not the events with the same DEVPATH, like "remove"
>>>>>beating an "add". But I'm not sure if the number isn't useful. Whatever
>>>>>we may do with the hotplug over netlink in the future, we will only have
>>>>>/sbin/hotplug for the early boot and it may be nice to know, what events
>>>>>we have already handled...
>>>>>
>>>>>
>>>>>>I'll hold off on applying this patch until we figure this out...
>>>>>
>>>>>How about just reserving 20 bytes for the number (u64 will never be
>>>>>more than that), save the pointer to that field, and fill the number in
>>>>>later?
>>>>
>>>>Ah, something like this instead?  I like it, it's even smaller than the
>>>>previous patch.  Compile tested only...
>>>
>>>I like that. How about the following. It will keep the buffer clean from
>>>random chars, cause the kevent does not have the vector and relies on
>>>the '\0' to separate the strings from each other.
>>>I've tested it. The netlink-hotplug message looks like this:
>>>
>>>recv(3, "remove@/class/input/mouse2\0ACTION=remove\0DEVPATH=/class/input/mouse2\0SUBSYSTEM=input\0SEQNUM=961                 \0", 1024, 0) = 113
>>
>>Hmm, these trailing spaces are just bad, sorry. I'll better pass the
>>envp array over to send_uevent() and clean up the keys while copying
>>the env values into the skb buffer. This will make the event payload
>>more safe too. So your first version looks better.
> 
> 
> How about this? We copy over key by key into the skb buffer and the
> netlink message can get the envp array without depending on a single
> continuous buffer.
> 
> The netlink message looks nice like this now:
> 
> recv(3, "
>   add@/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.0\0
>   HOME=/\0
>   PATH=/sbin:/bin:/usr/sbin:/usr/bin\0
>   ACTION=add\0
>   DEVPATH=/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.0\0
>   SUBSYSTEM=usb\0
>   SEQNUM=991\0
>   DEVICE=/proc/bus/usb/003/008\0
>   PRODUCT=46d/c03e/2000\0
>   TYPE=0/0/0\0
>   INTERFACE=3/1/2\0
> ", 1024, 0) = 268
> 

Yeah, sending the envp array instead of buffer to send_uevent() handles
all the cases. Great stuff - Thanks

Andrew

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Kay Sievers]

On Sat, Oct 30, 2004 at 04:54:29AM +0200, Kay Sievers wrote:
> On Sat, Oct 30, 2004 at 02:25:23AM +0200, Kay Sievers wrote:
> > On Sat, Oct 30, 2004 at 02:00:45AM +0200, Kay Sievers wrote:
> > > On Fri, Oct 29, 2004 at 06:13:19PM -0500, Greg KH wrote:
> > > > On Fri, Oct 29, 2004 at 11:28:56PM +0200, Kay Sievers wrote:
> > > > > > But there might still be a problem.  With this change, the sequence
> > > > > > number is not sent out the kevent message.  Kay, do you think this is an
> > > > > > issue?  I don't think we can get netlink messages out of order, right?
> > > > > 
> > > > > Right, especially not the events with the same DEVPATH, like "remove"
> > > > > beating an "add". But I'm not sure if the number isn't useful. Whatever
> > > > > we may do with the hotplug over netlink in the future, we will only have
> > > > > /sbin/hotplug for the early boot and it may be nice to know, what events
> > > > > we have already handled...
> > > > > 
> > > > > > I'll hold off on applying this patch until we figure this out...
> > > > > 
> > > > > How about just reserving 20 bytes for the number (u64 will never be
> > > > > more than that), save the pointer to that field, and fill the number in
> > > > > later?
> > > > 
> > > > Ah, something like this instead?  I like it, it's even smaller than the
> > > > previous patch.  Compile tested only...
> > > 
> > > I like that. How about the following. It will keep the buffer clean from
> > > random chars, cause the kevent does not have the vector and relies on
> > > the '\0' to separate the strings from each other.
> > > I've tested it. The netlink-hotplug message looks like this:
> > > 
> > > recv(3, "remove@/class/input/mouse2\0ACTION=remove\0DEVPATH=/class/input/mouse2\0SUBSYSTEM=input\0SEQNUM=961                 \0", 1024, 0) = 113
> > 
> > Hmm, these trailing spaces are just bad, sorry. I'll better pass the
> > envp array over to send_uevent() and clean up the keys while copying
> > the env values into the skb buffer. This will make the event payload
> > more safe too. So your first version looks better.
> 
> How about this? We copy over key by key into the skb buffer and the
> netlink message can get the envp array without depending on a single
> continuous buffer.
> 
> The netlink message looks nice like this now:
> 
> recv(3, "
>   add@/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.0\0
>   HOME=/\0
>   PATH=/sbin:/bin:/usr/sbin:/usr/bin\0
>   ACTION=add\0
>   DEVPATH=/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.0\0
>   SUBSYSTEM=usb\0
>   SEQNUM=991\0
>   DEVICE=/proc/bus/usb/003/008\0
>   PRODUCT=46d/c03e/2000\0
>   TYPE=0/0/0\0
>   INTERFACE=3/1/2\0
> ", 1024, 0) = 268

Here is an improved version that uses skb_put() to fill the skb buffer,
instead of trimming the buffer to the final size after we've copied over
all keys.

Thanks,
Kay


Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>

===== lib/kobject_uevent.c 1.10 vs edited =====
--- 1.10/lib/kobject_uevent.c	2004-10-23 00:42:52 +02:00
+++ edited/lib/kobject_uevent.c	2004-10-31 04:58:32 +01:00
@@ -23,6 +23,9 @@
 #include <linux/kobject.h>
 #include <net/sock.h>
 
+#define BUFFER_SIZE	1024	/* buffer for the hotplug env */
+#define NUM_ENVP	32	/* number of env pointers */
+
 #if defined(CONFIG_KOBJECT_UEVENT) || defined(CONFIG_HOTPLUG)
 static char *action_to_string(enum kobject_action action)
 {
@@ -53,12 +56,11 @@ static struct sock *uevent_sock;
  *
  * @signal: signal name
  * @obj: object path (kobject)
- * @buf: buffer used to pass auxiliary data like the hotplug environment
- * @buflen:
- * gfp_mask:
+ * @envp: possible hotplug environment to pass with the message
+ * @gfp_mask:
  */
-static int send_uevent(const char *signal, const char *obj, const void *buf,
-			int buflen, int gfp_mask)
+static int send_uevent(const char *signal, const char *obj,
+		       char **envp, int gfp_mask)
 {
 	struct sk_buff *skb;
 	char *pos;
@@ -69,16 +71,25 @@ static int send_uevent(const char *signa
 
 	len = strlen(signal) + 1;
 	len += strlen(obj) + 1;
-	len += buflen;
 
-	skb = alloc_skb(len, gfp_mask);
+	/* allocate buffer with the maximum possible message size */
+	skb = alloc_skb(len + BUFFER_SIZE, gfp_mask);
 	if (!skb)
 		return -ENOMEM;
 
 	pos = skb_put(skb, len);
+	sprintf(pos, "%s@%s", signal, obj);
 
-	pos += sprintf(pos, "%s@%s", signal, obj) + 1;
-	memcpy(pos, buf, buflen);
+	/* copy the environment key by key to our continuous buffer */
+	if (envp) {
+		int i;
+
+		for (i = 2; envp[i]; i++) {
+			len = strlen(envp[i]) + 1;
+			pos = skb_put(skb, len);
+			strcpy(pos, envp[i]);
+		}
+	}
 
 	return netlink_broadcast(uevent_sock, skb, 0, 1, gfp_mask);
 }
@@ -107,10 +118,10 @@ static int do_kobject_uevent(struct kobj
 		if (!attrpath)
 			goto exit;
 		sprintf(attrpath, "%s/%s", path, attr->name);
-		rc = send_uevent(signal, attrpath, NULL, 0, gfp_mask);
+		rc = send_uevent(signal, attrpath, NULL, gfp_mask);
 		kfree(attrpath);
 	} else {
-		rc = send_uevent(signal, path, NULL, 0, gfp_mask);
+		rc = send_uevent(signal, path, NULL, gfp_mask);
 	}
 
 exit:
@@ -169,8 +180,6 @@ static inline int send_uevent(const char
 u64 hotplug_seqnum;
 static spinlock_t sequence_lock = SPIN_LOCK_UNLOCKED;
 
-#define BUFFER_SIZE	1024	/* should be enough memory for the env */
-#define NUM_ENVP	32	/* number of env pointers */
 /**
  * kobject_hotplug - notify userspace by executing /sbin/hotplug
  *
@@ -182,6 +191,7 @@ void kobject_hotplug(struct kobject *kob
 	char *argv [3];
 	char **envp = NULL;
 	char *buffer = NULL;
+	char *seq_buff;
 	char *scratch;
 	int i = 0;
 	int retval;
@@ -258,6 +268,11 @@ void kobject_hotplug(struct kobject *kob
 	envp [i++] = scratch;
 	scratch += sprintf(scratch, "SUBSYSTEM=%s", name) + 1;
 
+	/* reserve space for the sequence,
+	 * put the real one in after the hotplug call */
+	envp[i++] = seq_buff = scratch;
+	scratch += strlen("SEQNUM=18446744073709551616") + 1;
+
 	if (hotplug_ops->hotplug) {
 		/* have the kset specific function add its stuff */
 		retval = hotplug_ops->hotplug (kset, kobj,
@@ -273,15 +288,13 @@ void kobject_hotplug(struct kobject *kob
 	spin_lock(&sequence_lock);
 	seq = ++hotplug_seqnum;
 	spin_unlock(&sequence_lock);
-
-	envp [i++] = scratch;
-	scratch += sprintf(scratch, "SEQNUM=%lld", (long long)seq) + 1;
+	sprintf(seq_buff, "SEQNUM=%lld", (long long)seq);
 
 	pr_debug ("%s: %s %s seq=%lld %s %s %s %s %s\n",
 		  __FUNCTION__, argv[0], argv[1], (long long)seq,
 		  envp[0], envp[1], envp[2], envp[3], envp[4]);
 
-	send_uevent(action_string, kobj_path, buffer, scratch - buffer, GFP_KERNEL);
+	send_uevent(action_string, kobj_path, envp, GFP_KERNEL);
 
 	if (!hotplug_path[0])
 		goto exit;

Re: [Patch] 2.6.10.rc1.bk6 /lib/kobject_uevent.c buffer issues

[Greg KH]

On Sun, Oct 31, 2004 at 05:11:12AM +0100, Kay Sievers wrote:
> On Sat, Oct 30, 2004 at 04:54:29AM +0200, Kay Sievers wrote:
> > On Sat, Oct 30, 2004 at 02:25:23AM +0200, Kay Sievers wrote:
> > > On Sat, Oct 30, 2004 at 02:00:45AM +0200, Kay Sievers wrote:
> > > > On Fri, Oct 29, 2004 at 06:13:19PM -0500, Greg KH wrote:
> > > > > On Fri, Oct 29, 2004 at 11:28:56PM +0200, Kay Sievers wrote:
> > > > > > > But there might still be a problem.  With this change, the sequence
> > > > > > > number is not sent out the kevent message.  Kay, do you think this is an
> > > > > > > issue?  I don't think we can get netlink messages out of order, right?
> > > > > > 
> > > > > > Right, especially not the events with the same DEVPATH, like "remove"
> > > > > > beating an "add". But I'm not sure if the number isn't useful. Whatever
> > > > > > we may do with the hotplug over netlink in the future, we will only have
> > > > > > /sbin/hotplug for the early boot and it may be nice to know, what events
> > > > > > we have already handled...
> > > > > > 
> > > > > > > I'll hold off on applying this patch until we figure this out...
> > > > > > 
> > > > > > How about just reserving 20 bytes for the number (u64 will never be
> > > > > > more than that), save the pointer to that field, and fill the number in
> > > > > > later?
> > > > > 
> > > > > Ah, something like this instead?  I like it, it's even smaller than the
> > > > > previous patch.  Compile tested only...
> > > > 
> > > > I like that. How about the following. It will keep the buffer clean from
> > > > random chars, cause the kevent does not have the vector and relies on
> > > > the '\0' to separate the strings from each other.
> > > > I've tested it. The netlink-hotplug message looks like this:
> > > > 
> > > > recv(3, "remove@/class/input/mouse2\0ACTION=remove\0DEVPATH=/class/input/mouse2\0SUBSYSTEM=input\0SEQNUM=961                 \0", 1024, 0) = 113
> > > 
> > > Hmm, these trailing spaces are just bad, sorry. I'll better pass the
> > > envp array over to send_uevent() and clean up the keys while copying
> > > the env values into the skb buffer. This will make the event payload
> > > more safe too. So your first version looks better.
> > 
> > How about this? We copy over key by key into the skb buffer and the
> > netlink message can get the envp array without depending on a single
> > continuous buffer.
> > 
> > The netlink message looks nice like this now:
> > 
> > recv(3, "
> >   add@/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.0\0
> >   HOME=/\0
> >   PATH=/sbin:/bin:/usr/sbin:/usr/bin\0
> >   ACTION=add\0
> >   DEVPATH=/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.0\0
> >   SUBSYSTEM=usb\0
> >   SEQNUM=991\0
> >   DEVICE=/proc/bus/usb/003/008\0
> >   PRODUCT=46d/c03e/2000\0
> >   TYPE=0/0/0\0
> >   INTERFACE=3/1/2\0
> > ", 1024, 0) = 268
> 
> Here is an improved version that uses skb_put() to fill the skb buffer,
> instead of trimming the buffer to the final size after we've copied over
> all keys.

Thanks, I've applied this version.  Hopefully I'll get it out to Linus
later today.

greg k-h