[PATCH] [-mm] ACPI: export ACPI events via netlink

[PATCH] [-mm] ACPI: export ACPI events via netlink

[Zhang Rui]

From: Zhang Rui <rui.zhang@intel.com>

Export ACPI events via netlink.
A netlink message is broadcasted when an ACPI event is generated.

Note: The behaviour of how ACPI event works nowadays is not changed.
	Netlink is used to export ACPI event instead of /proc/acpi/event someday,
	but not now.
	This patch only adds the function of sending netlink messages
	when an ACPI event is generated.
	Following is an example of how to receive ACPI event messages.

#include <linux/socket.h>
#include <linux/netlink.h>
#include "stdio.h"

#define NETLINK_ACPI_EVENT	20
#define SOCK_RAW		3

struct acpi_event{
	char device_class[20];
	char bus_id[15];
	unsigned int type;
	unsigned int data;
};

struct sockaddr_nl src_addr, dest_addr;
int sock_fd;
struct msghdr msg;
struct iovec iov;


int main (void){
	int i = 10;
	int result;
	struct acpi_event event;

	sock_fd = socket(PF_NETLINK, SOCK_RAW, NETLINK_ACPI_EVENT);
	if (sock_fd == -1) {
		printf("Socket faied!\n");
		return 0;
	}

	src_addr.nl_family = AF_NETLINK;
	src_addr.nl_pid = getpid();

	src_addr.nl_groups = 1;

	result = bind(sock_fd, (struct sockaddr*)&src_addr, sizeof(src_addr));
	if (result) {
		printf("Bind faied! %d.\n", result);
		return result;
	}

	iov.iov_base = (void *)&event;
	iov.iov_len = sizeof(struct acpi_event);

	msg.msg_name = (void *)&dest_addr;
	msg.msg_namelen = sizeof(dest_addr);
	msg.msg_iov = &iov;
	msg.msg_iovlen = 1;

	while(i > 0) {
		printf("Wait...\n");
		result = recvmsg(sock_fd, &msg, 0);
		if (result == -1) {
			printf("Rui: recvmsg failed, error is %d\n", result);
			return result;
		}
		printf("%20s %15s %08x %08x\n",
			event.device_class, event.bus_id, event.type, event.data);
		i--;
	}

	close(sock_fd);
	return 0;
}

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/bus.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/event.c    |   25 +++++++++++++++++++++++++
 include/acpi/acpi_bus.h |    4 +++-
 include/linux/netlink.h |    1 +
 4 files changed, 71 insertions(+), 1 deletion(-)

Index: linux-2.6.22-rc1/drivers/acpi/bus.c
===================================================================
--- linux-2.6.22-rc1.orig/drivers/acpi/bus.c	2007-05-21 10:18:58.000000000 +0800
+++ linux-2.6.22-rc1/drivers/acpi/bus.c	2007-05-21 15:38:06.000000000 +0800
@@ -37,6 +37,7 @@
 #endif
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
+#include <linux/netlink.h>
 
 #define _COMPONENT		ACPI_BUS_COMPONENT
 ACPI_MODULE_NAME("bus");
@@ -275,6 +276,43 @@
 /* --------------------------------------------------------------------------
                                 Event Management
    -------------------------------------------------------------------------- */
+#ifdef CONFIG_NET
+struct acpi_bus_netlink_event {
+	acpi_device_class device_class;
+	char bus_id[15];
+	u32 type;
+	u32 data;
+};
+
+static int acpi_bus_generate_netlink_event(struct acpi_device *device,
+						u8 type, int data)
+{
+	struct sk_buff *skb = NULL;
+	struct acpi_bus_netlink_event *event = NULL;
+
+	skb = alloc_skb(sizeof(struct acpi_bus_event), GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	event = (struct acpi_bus_netlink_event *)
+			skb_put(skb, sizeof(struct acpi_bus_netlink_event));
+	strcpy(event->device_class, device->pnp.device_class);
+	strcpy(event->bus_id, device->dev.bus_id);
+	event->type = type;
+	event->data = data;
+
+	NETLINK_CB(skb).dst_group = 1;
+
+	netlink_broadcast(acpi_event_sock, skb, 0, 1, GFP_ATOMIC);
+	return 0;
+}
+#else
+static int acpi_bus_generate_netlink_event(struct acpi_device *device,
+						u8 type, int data)
+{
+	return 0;
+}
+#endif
 
 static DEFINE_SPINLOCK(acpi_bus_event_lock);
 
@@ -292,6 +330,10 @@
 	if (!device)
 		return -EINVAL;
 
+	if (acpi_bus_generate_netlink_event(device, type, data))
+		printk(KERN_WARNING PREFIX
+			"Failed to generate a netlink message for ACPI event!\n");
+
 	/* drop event on the floor if no one's listening */
 	if (!event_is_open)
 		return 0;
Index: linux-2.6.22-rc1/drivers/acpi/event.c
===================================================================
--- linux-2.6.22-rc1.orig/drivers/acpi/event.c	2007-05-16 16:12:46.000000000 +0800
+++ linux-2.6.22-rc1/drivers/acpi/event.c	2007-05-21 15:38:32.000000000 +0800
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/poll.h>
 #include <acpi/acpi_drivers.h>
+#include <linux/netlink.h>
 
 #define _COMPONENT		ACPI_SYSTEM_COMPONENT
 ACPI_MODULE_NAME("event");
@@ -105,6 +106,24 @@
 	.release = acpi_system_close_event,
 	.poll = acpi_system_poll_event,
 };
+#ifdef CONFIG_NET
+struct sock* acpi_event_sock;
+static int acpi_event_netlink_init(void)
+{
+	acpi_event_sock = netlink_kernel_create(NETLINK_ACPI_EVENT, 1, NULL,
+						NULL, THIS_MODULE);
+
+	if (!acpi_event_sock)
+		return -ENODEV;
+
+	return 0;
+}
+#else
+static int acpi_event_netlink_init(void)
+{
+	return -ENODEV;
+}
+#endif
 
 static int __init acpi_event_init(void)
 {
@@ -115,6 +134,12 @@
 	if (acpi_disabled)
 		return 0;
 
+	/* create netlink for acpi event */
+	error = acpi_event_netlink_init();
+	if (error)
+		printk(KERN_WARNING PREFIX
+		       "Failed to create netlink for ACPI event\n");
+
 	/* 'event' [R] */
 	entry = create_proc_entry("event", S_IRUSR, acpi_root_dir);
 	if (entry)
Index: linux-2.6.22-rc1/include/linux/netlink.h
===================================================================
--- linux-2.6.22-rc1.orig/include/linux/netlink.h	2007-05-21 10:19:00.000000000 +0800
+++ linux-2.6.22-rc1/include/linux/netlink.h	2007-05-21 15:26:14.000000000 +0800
@@ -24,6 +24,7 @@
 /* leave room for NETLINK_DM (DM Events) */
 #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
 #define NETLINK_ECRYPTFS	19
+#define NETLINK_ACPI_EVENT	20	/* acpi event notifications */
 
 #define MAX_LINKS 32		
 
Index: linux-2.6.22-rc1/include/acpi/acpi_bus.h
===================================================================
--- linux-2.6.22-rc1.orig/include/acpi/acpi_bus.h	2007-05-21 10:19:00.000000000 +0800
+++ linux-2.6.22-rc1/include/acpi/acpi_bus.h	2007-05-21 15:34:36.000000000 +0800
@@ -321,7 +321,9 @@
 };
 
 extern struct kset acpi_subsys;
-
+#ifdef CONFIG_NET
+extern struct sock* acpi_event_sock;
+#endif
 /*
  * External Functions
  */

Re: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Samuel Ortiz]

On 5/22/2007, "Zhang Rui" <rui.zhang@intel.com> wrote:
>Index: linux-2.6.22-rc1/include/linux/netlink.h
>===================================================================
>--- linux-2.6.22-rc1.orig/include/linux/netlink.h	2007-05-21 10:19:00.000000000 +0800
>+++ linux-2.6.22-rc1/include/linux/netlink.h	2007-05-21 15:26:14.000000000 +0800
>@@ -24,6 +24,7 @@
> /* leave room for NETLINK_DM (DM Events) */
> #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
> #define NETLINK_ECRYPTFS	19
>+#define NETLINK_ACPI_EVENT	20	/* acpi event notifications */
I think it is recommended to use the generic netlink layer instead of
having everyone adding its own netlink ID.

Cheers,
Samuel.

Re: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Evgeniy Polyakov]

On Tue, May 22, 2007 at 10:05:00AM -0000, Samuel Ortiz (samuel@sortiz.org) wrote:
> 
> On 5/22/2007, "Zhang Rui" <rui.zhang@intel.com> wrote:
> >Index: linux-2.6.22-rc1/include/linux/netlink.h
> >===================================================================
> >--- linux-2.6.22-rc1.orig/include/linux/netlink.h	2007-05-21 10:19:00.000000000 +0800
> >+++ linux-2.6.22-rc1/include/linux/netlink.h	2007-05-21 15:26:14.000000000 +0800
> >@@ -24,6 +24,7 @@
> > /* leave room for NETLINK_DM (DM Events) */
> > #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
> > #define NETLINK_ECRYPTFS	19
> >+#define NETLINK_ACPI_EVENT	20	/* acpi event notifications */
> I think it is recommended to use the generic netlink layer instead of
> having everyone adding its own netlink ID.

It is possible to allocate your own netlink protocol number, but Samuel
is right, it is better to use existing delivery mechanisms like
genetlink and connector.

> Cheers,
> Samuel.
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
	Evgeniy Polyakov

Re: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

Hi Zhang Rui,

Really cool stuff. Can you instead use genetlink?
http://linux-net.osdl.org/index.php/Generic_Netlink_HOWTO 
should help. And if you have more questions post on netdev (not lk).

cheers,
jamal

On Tue, 2007-22-05 at 17:47 +0800, Zhang Rui wrote:
> From: Zhang Rui <rui.zhang@intel.com>
> 
> Export ACPI events via netlink.
> A netlink message is broadcasted when an ACPI event is generated.
> 

Re: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Zhang Rui]

On Tue, 2007-05-22 at 07:03 -0400, jamal wrote:
> Hi Zhang Rui,
> 
> Really cool stuff. Can you instead use genetlink?
> http://linux-net.osdl.org/index.php/Generic_Netlink_HOWTO 
> should help. And if you have more questions post on netdev (not lk).
> 
That's really helpful, thanks.
Will post the second version soon. :)

Thanks,
Rui

> On Tue, 2007-22-05 at 17:47 +0800, Zhang Rui wrote:
> > From: Zhang Rui <rui.zhang@intel.com>
> > 
> > Export ACPI events via netlink.
> > A netlink message is broadcasted when an ACPI event is generated.
> > 

Re: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Zhang Rui]

On Tue, 2007-05-22 at 07:03 -0400, jamal wrote:
> Hi Zhang Rui,
> 
> Really cool stuff. Can you instead use genetlink?
> http://linux-net.osdl.org/index.php/Generic_Netlink_HOWTO 
> should help. And if you have more questions post on netdev (not lk).
> 
Yes, I have one now. :)
I need to write a user application to test my patch.
Netlink messages can be sent/received using the standard socket API.
But how to receive Genetlink messages from specified genetlink family?
There is no socket ACPI with such a parameter, right?
Do I have to receive all the genetlink messages first?
It would be great if there are any examples on user space communication.

Or should I use libnl library instead?

Thanks,
Rui.

Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

I can never get these web-browser email clients to work well.
Sorry for sending fscking html earlier.

cheers,
jamal

---------- Forwarded message ----------
From: jamal <hadi@cyberus.ca>
Date: May 27, 2007 9:29 AM
Subject: Re: [PATCH] [-mm] ACPI: export ACPI events via netlink
To: Zhang Rui <rui.zhang@intel.com>
Cc: linux-acpi@vger.kernel.org, netdev@vger.kernel.org




On 5/27/07, Zhang Rui <rui.zhang@intel.com> wrote:
>
> I need to write a user application to test my patch.
> Netlink messages can be sent/received using the standard socket API.

sure.

> But how to receive Genetlink messages from specified genetlink family?
> There is no socket ACPI with such a parameter, right?

Each module has a unique identifier that it receives dynamically on
 insertion at the kernel.

> Do I have to receive all the genetlink messages first?

 No, just the ones for your dynamic id. Try what i described first for
 kernel side on the earlier email. I will repeat it here for clarity.
Then look at genl code and if you have questions i can
 help.
Note: You need to discover your dynamic id (the iproute2/genl code has a stub
example code)
 As i told you in the earlier email, in your development:
 - start first by just writting your kernel side.
 - Then use the genl utility - which is part of iproute2 to see if the
 kernel side is "discoverable".

 E.g if i wanted to "discover" currently loaded modules on my laptop, i
 would do this:

 ---------------
 hadi@lilsol:~$ genl ctrl ls

 Name: nlctrl
         ID: 0x10  Version: 0x2  header size: 0  max attribs: 6
         commands supported:
                 #1:  ID-0x3  flags-0xe


 Name: nl80211
         ID: 0x11  Version: 0x1  header size: 0  max attribs: 22
         commands supported:
                 #1:  ID-0x1  flags-0xa
                 #2:  ID-0x6  flags-0xa
                 #3:  ID-0x8  flags-0xa
                 #4:  ID-0x3  flags-0xb
                 #5:  ID-0x4  flags-0xb
                 #6:  ID-0x5  flags-0xb
                 #7:  ID-0xa  flags-0xb
                 #8:  ID-0xb  flags-0xa
                 #9:  ID-0xf  flags-0xb
                 #10:  ID-0x10  flags-0xa
                 #11:  ID-0x12  flags-0xb
                 #12:  ID-0x13  flags-0xa
                 #13:  ID-0x15  flags-0xa
                 #14:  ID-0x19  flags-0xb
                 #15:  ID-0x17  flags-0xb
                 #16:  ID-0x18  flags-0xb
                 #17:  ID-0x1a  flags-0xb
                 #18:  ID-0x1b  flags-0xa
                 #19:  ID-0xd  flags-0xb


 Name: TASKSTATS
         ID: 0x12  Version: 0x1  header size: 0  max attribs: 4
         commands supported:
                 #1:  ID-0x1  flags-0xa
 ---------------------------

 As you can see, i can see from user space the name of the kernel end
 point, its numeric id, what version it is running (so i can make sure
 user space is compatible), what extra header it may have, what the
 maximum number of attributes it can take. The last thing that gets
 listed is the commands, and flags for those commands.

 Lets load tipc kernel module and repeat...

 -----------

 hadi@lilsol:~$ sudo modprobe tipc
 Name: nlctrl
         ID: 0x10  Version: 0x2  header size: 0  max attribs: 6
         commands supported:
                 #1:  ID-0x3  flags-0xe

 ....
 [same as before]
 ....

 Name: TIPC
         ID: 0x13  Version: 0x1  header size: 8  max attribs: 0
         commands supported:
                 #1:  ID-0x1  flags-0x2

 ===============

> It would be great if there are any examples on user space communication.



Bug Thomas - he has written some simple example. I also have some but i
 changed laptops and i have to go and dig it up for you.
 I do have plans for making this easier for people - but havent had time.
 If there is persistence - or someone out there wants to be a hero email
 me privately and i will explain it.

> Or should I use libnl library instead?

Why am i answering all these questions if you are fine with using libnl?
Last time you said you couldnt use a library, no?

cheers,
jamal


> Thanks,
> Rui.
>

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Zhang Rui]

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

Hi, Jamal,

Now the genl utility can find the acpi event genetlink family.
And a simple user space demo is finished for handling acpi event.
I really appreciate your help. :)

I think the patch which exposes ACPI events via netlink is ok.
But I still have some problems on
how to listen to specified genetlink family in user space?

I can get the dynamic id for "acpi_event" genl family.
But I don't know how to use this to receive messages from
specified genl family.
It seems that "#genl ctrl monitor" has something to do with this,
IMO, rtnl_open_byproto(&rth, nl_mgrp(GENL_ID_CTRL), NETLINK_GENERIC) is
used to receive messages from the nlctrl(controller) only, but
unfortunately it never works for me. :(

Any suggestions? What interfaces should I use? Or where can I find some
example code?

Attachment is the simple user space demo I made.
It receives all the broadcasted genetlink messages and only parses the
ones sent by "acpi_event" genl family.


Thanks,
Rui

On Sun, 2007-05-27 at 09:34 -0400, jamal wrote: 
> On 5/27/07, Zhang Rui <rui.zhang@intel.com> wrote:
> >
> > I need to write a user application to test my patch.
> > Netlink messages can be sent/received using the standard socket API.
> 
> sure.
> 
> > But how to receive Genetlink messages from specified genetlink family?
> > There is no socket ACPI with such a parameter, right?
> 
> Each module has a unique identifier that it receives dynamically on
>  insertion at the kernel.
> 
> > Do I have to receive all the genetlink messages first?
> 
>  No, just the ones for your dynamic id. Try what i described first for
>  kernel side on the earlier email. I will repeat it here for clarity.
> Then look at genl code and if you have questions i can
>  help.
> Note: You need to discover your dynamic id (the iproute2/genl code has a stub
> example code)
>  As i told you in the earlier email, in your development:
>  - start first by just writting your kernel side.
>  - Then use the genl utility - which is part of iproute2 to see if the
>  kernel side is "discoverable".
> 
>  E.g if i wanted to "discover" currently loaded modules on my laptop, i
>  would do this:
> 
>  ---------------
>  hadi@lilsol:~$ genl ctrl ls
> 
>  Name: nlctrl
>          ID: 0x10  Version: 0x2  header size: 0  max attribs: 6
>          commands supported:
>                  #1:  ID-0x3  flags-0xe
> 
> 
>  Name: nl80211
>          ID: 0x11  Version: 0x1  header size: 0  max attribs: 22
>          commands supported:
>                  #1:  ID-0x1  flags-0xa
>                  #2:  ID-0x6  flags-0xa
>                  #3:  ID-0x8  flags-0xa
>                  #4:  ID-0x3  flags-0xb
>                  #5:  ID-0x4  flags-0xb
>                  #6:  ID-0x5  flags-0xb
>                  #7:  ID-0xa  flags-0xb
>                  #8:  ID-0xb  flags-0xa
>                  #9:  ID-0xf  flags-0xb
>                  #10:  ID-0x10  flags-0xa
>                  #11:  ID-0x12  flags-0xb
>                  #12:  ID-0x13  flags-0xa
>                  #13:  ID-0x15  flags-0xa
>                  #14:  ID-0x19  flags-0xb
>                  #15:  ID-0x17  flags-0xb
>                  #16:  ID-0x18  flags-0xb
>                  #17:  ID-0x1a  flags-0xb
>                  #18:  ID-0x1b  flags-0xa
>                  #19:  ID-0xd  flags-0xb
> 
> 
>  Name: TASKSTATS
>          ID: 0x12  Version: 0x1  header size: 0  max attribs: 4
>          commands supported:
>                  #1:  ID-0x1  flags-0xa
>  ---------------------------
> 
>  As you can see, i can see from user space the name of the kernel end
>  point, its numeric id, what version it is running (so i can make sure
>  user space is compatible), what extra header it may have, what the
>  maximum number of attributes it can take. The last thing that gets
>  listed is the commands, and flags for those commands.
> 
>  Lets load tipc kernel module and repeat...
> 
>  -----------
> 
>  hadi@lilsol:~$ sudo modprobe tipc
>  Name: nlctrl
>          ID: 0x10  Version: 0x2  header size: 0  max attribs: 6
>          commands supported:
>                  #1:  ID-0x3  flags-0xe
> 
>  ....
>  [same as before]
>  ....
> 
>  Name: TIPC
>          ID: 0x13  Version: 0x1  header size: 8  max attribs: 0
>          commands supported:
>                  #1:  ID-0x1  flags-0x2
> 
>  ===============
> 
> > It would be great if there are any examples on user space communication.
> 
> 
> 
> Bug Thomas - he has written some simple example. I also have some but i
>  changed laptops and i have to go and dig it up for you.
>  I do have plans for making this easier for people - but havent had time.
>  If there is persistence - or someone out there wants to be a hero email
>  me privately and i will explain it.
> 
> > Or should I use libnl library instead?
> 
> Why am i answering all these questions if you are fine with using libnl?
> Last time you said you couldnt use a library, no?
> 
> cheers,
> jamal
> 
> 
> > Thanks,
> > Rui.
> >

[-- Attachment #2: acpi_genl.tgz --]
[-- Type: application/x-compressed-tar, Size: 8401 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Thu, 2007-14-06 at 16:59 +0800, Zhang Rui wrote:
> Hi, Jamal,
> 
> Now the genl utility can find the acpi event genetlink family.
> And a simple user space demo is finished for handling acpi event.
> I really appreciate your help. :)

np.

> I think the patch which exposes ACPI events via netlink is ok.
> But I still have some problems on
> how to listen to specified genetlink family in user space?
> 
> I can get the dynamic id for "acpi_event" genl family.
> But I don't know how to use this to receive messages from
> specified genl family.
> It seems that "#genl ctrl monitor" has something to do with this,
> IMO, rtnl_open_byproto(&rth, nl_mgrp(GENL_ID_CTRL), NETLINK_GENERIC) is
> used to receive messages from the nlctrl(controller) only, but
> unfortunately it never works for me. :(
> 

I dont have much time to look at your code given travel, but did you
try to use your group id instead of the controller's?
i.e:
rtnl_open_byproto(&rth, nl_mgrp(mydiscoveredacpiid), NETLINK_GENERIC)

If this doesnt work, ping me and i will take a look  - just expect some
latency in response.

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Zhang Rui]

On Thu, 2007-06-14 at 07:28 -0400, jamal wrote:
> On Thu, 2007-14-06 at 16:59 +0800, Zhang Rui wrote:
> > Hi, Jamal,
> > 
> > Now the genl utility can find the acpi event genetlink family.
> > And a simple user space demo is finished for handling acpi event.
> > I really appreciate your help. :)
> 
> np.
> 
> > I think the patch which exposes ACPI events via netlink is ok.
> > But I still have some problems on
> > how to listen to specified genetlink family in user space?
> > 
> > I can get the dynamic id for "acpi_event" genl family.
> > But I don't know how to use this to receive messages from
> > specified genl family.
> > It seems that "#genl ctrl monitor" has something to do with this,
> > IMO, rtnl_open_byproto(&rth, nl_mgrp(GENL_ID_CTRL), NETLINK_GENERIC) is
> > used to receive messages from the nlctrl(controller) only, but
> > unfortunately it never works for me. :(
> > 
> 
> I dont have much time to look at your code given travel, but did you
> try to use your group id instead of the controller's?
> i.e:
> rtnl_open_byproto(&rth, nl_mgrp(mydiscoveredacpiid), NETLINK_GENERIC)
> 
Yes. It doesn't work if I use my group id here.
In fact, I'm using rtnl_open_byproto(&rth, 1, NETLINK_GENERIC) now.
That's why I said that this demo receives all the broadcasted genetlink
messages.
> If this doesnt work, ping me and i will take a look  - just expect some
> latency in response.
> 
That's great. Thanks.

Best regards,
Rui

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-15-06 at 09:01 +0800, Zhang Rui wrote:

> > rtnl_open_byproto(&rth, nl_mgrp(mydiscoveredacpiid), NETLINK_GENERIC)
> > 
> Yes. It doesn't work if I use my group id here.
> In fact, I'm using rtnl_open_byproto(&rth, 1, NETLINK_GENERIC) now.
> That's why I said that this demo receives all the broadcasted genetlink
> messages.

Ok, tell me how to generate these ACPI events and i will patch my laptop
and test it. What kernel compile options do i need? 2.6.24-rc4 will do?

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-15-06 at 09:01 +0800, Zhang Rui wrote:

> > I dont have much time to look at your code given travel, but did you
> > try to use your group id instead of the controller's?
> > i.e:
> > rtnl_open_byproto(&rth, nl_mgrp(mydiscoveredacpiid), NETLINK_GENERIC)
> > 
> Yes. It doesn't work if I use my group id here.
> In fact, I'm using rtnl_open_byproto(&rth, 1, NETLINK_GENERIC) now.
> That's why I said that this demo receives all the broadcasted genetlink
> messages.

Ok, by inspection (sorry, still dont have much time) - your kernel code
is sending to group 1; i.e

genlmsg_multicast(skb, 0, 1, GFP_ATOMIC);

you need to change that to send to your assigned id, i.e:
genlmsg_multicast(skb, 0, acpi_event_genl_family.id, GFP_ATOMIC);

then the user space code will work. I should be able to look at it if it
doesnt work by end of week.

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Zhang Rui]

On Mon, 2007-06-18 at 11:01 -0400, jamal wrote:
> On Fri, 2007-15-06 at 09:01 +0800, Zhang Rui wrote:
> 
> > > I dont have much time to look at your code given travel, but did you
> > > try to use your group id instead of the controller's?
> > > i.e:
> > > rtnl_open_byproto(&rth, nl_mgrp(mydiscoveredacpiid), NETLINK_GENERIC)
> > > 
> > Yes. It doesn't work if I use my group id here.
> > In fact, I'm using rtnl_open_byproto(&rth, 1, NETLINK_GENERIC) now.
> > That's why I said that this demo receives all the broadcasted genetlink
> > messages.
> 
> Ok, by inspection (sorry, still dont have much time) - your kernel code
> is sending to group 1; i.e
> 
> genlmsg_multicast(skb, 0, 1, GFP_ATOMIC);
> 
> you need to change that to send to your assigned id, i.e:
> genlmsg_multicast(skb, 0, acpi_event_genl_family.id, GFP_ATOMIC);
> 
Oh, that's the problem.
Great, now it works happily. :).
Jamal, thanks for your help!

Best regards,
Rui

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Mon, 2007-06-18 at 11:01 -0400, jamal wrote:

> Ok, by inspection (sorry, still dont have much time) - your kernel code
> is sending to group 1; i.e
> 
> genlmsg_multicast(skb, 0, 1, GFP_ATOMIC);
> 
> you need to change that to send to your assigned id, i.e:
> genlmsg_multicast(skb, 0, acpi_event_genl_family.id, GFP_ATOMIC);
> 
> then the user space code will work. I should be able to look at it if it
> doesnt work by end of week.

Ah, that coincides with something I was wondering about. Isn't it
possible to have multiple multicast groups with generic netlink? If so,
we might have to use real netlink for wireless...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Tue, 2007-19-06 at 13:30 +0200, Johannes Berg wrote:

> Ah, that coincides with something I was wondering about. Isn't it
> possible to have multiple multicast groups with generic netlink? If so,
> we might have to use real netlink for wireless...


There is one default mcast group per entity. Most users only need that
one.
If you need more, we go one of two ways:
a) Register for multiple IDs with the code as is. 
b) we could add a "reserve multicast group" interface in the kernel.

Thoughts?

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

[took off acpi list]

On Tue, 2007-06-19 at 12:20 -0400, jamal wrote:

> There is one default mcast group per entity. Most users only need that
> one.

Ok. That's definitely a bug in nl80211 as we have it in development
right now. Btw, what happens if the group id gets larger than 31?

> If you need more, we go one of two ways:
> a) Register for multiple IDs with the code as is. 
> b) we could add a "reserve multicast group" interface in the kernel.
> 
> Thoughts?

I'd really like to be able to reserve multicast groups with special
semantics too, especially I might want to permit/deny non-CAP_NET_ADMIN
users from binding specific multicast groups. That isn't actually
possible with netlink nor genetlink right now afaict.

If we register multiple IDs then we'll end up filling up the generic
netlink family space really soon. I was under the impression that
generic netlink was basically open-ended because the family is a large
enough number, but with this arbitrary limit on multicast groups that's
really not true and we might run out of multicast groups fairly soon
since most users of generic netlink will want at least one...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Wed, 2007-20-06 at 13:25 +0200, Johannes Berg wrote:

> Ok. That's definitely a bug in nl80211 as we have it in development
> right now. 

Sorry, have never looked at that code.

> Btw, what happens if the group id gets larger than 31?

You can use setsockopt to set the multicast groups. What you cant do
with that is subscribe to many groups in one shot.
The call in iproute2 hasnt reflected this reality yet.

> I'd really like to be able to reserve multicast groups with special
> semantics too, especially I might want to permit/deny non-CAP_NET_ADMIN
> users from binding specific multicast groups. That isn't actually
> possible with netlink nor genetlink right now afaict.

This would be hard - but doable via SELinux interface. I think you
should be able to extend your tool to make calls to that interface.

> If we register multiple IDs then we'll end up filling up the generic
> netlink family space really soon. 

Theres a huge number of these groups; and not just that, but considering
that some genetlink users may not be interested in such multicast
groups, it is quiet usable to have many groups as long as we avoid
conflict.

> I was under the impression that
> generic netlink was basically open-ended because the family is a large
> enough number, but with this arbitrary limit on multicast groups that's
> really not true and we might run out of multicast groups fairly soon
> since most users of generic netlink will want at least one...
> 

The multicast issue wasnt well-attacked. We have a group magically
assigned to a user based on their allocated id. It should be feasible
to add an API to the kernel for registering for many groups and allow
user space to discover these groups before registering. Maybe thats
the path to proceed to.

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Thu, 2007-06-21 at 11:47 -0400, jamal wrote:
> On Wed, 2007-20-06 at 13:25 +0200, Johannes Berg wrote:
> 
> > Ok. That's definitely a bug in nl80211 as we have it in development
> > right now. 
> 
> Sorry, have never looked at that code.

No worries, I was just stating that.

> You can use setsockopt to set the multicast groups. What you cant do
> with that is subscribe to many groups in one shot.
> The call in iproute2 hasnt reflected this reality yet.

Ah, ok, I see now. I was under the impression that groups was always
just a u32.

> > I'd really like to be able to reserve multicast groups with special
> > semantics too, especially I might want to permit/deny non-CAP_NET_ADMIN
> > users from binding specific multicast groups. That isn't actually
> > possible with netlink nor genetlink right now afaict.
> 
> This would be hard - but doable via SELinux interface. I think you
> should be able to extend your tool to make calls to that interface.

Why do you think that would be hard? It'd basically just mean replacing
the netlink_capable(sock, NL_NONROOT_RECV) calls with a call that
actually tests depending on the group(s) it wants.

> > If we register multiple IDs then we'll end up filling up the generic
> > netlink family space really soon. 
> 
> Theres a huge number of these groups; and not just that, but considering
> that some genetlink users may not be interested in such multicast
> groups, it is quiet usable to have many groups as long as we avoid
> conflict.

Yeah, never mind, I thought that the number of groups was limited to 32.

> The multicast issue wasnt well-attacked. We have a group magically
> assigned to a user based on their allocated id. It should be feasible
> to add an API to the kernel for registering for many groups and allow
> user space to discover these groups before registering. Maybe thats
> the path to proceed to.

Yeah, sounds reasonable, you could ask the controller for which groups
are attached to a family and then get the IDs for those groups by name.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-22-06 at 12:09 +0200, Johannes Berg wrote:

> Why do you think that would be hard? It'd basically just mean replacing
> the netlink_capable(sock, NL_NONROOT_RECV) calls with a call that
> actually tests depending on the group(s) it wants.

I think it could be done. You will need to have root maybe initially set
such permissions etc - but it may be overkill.

> Yeah, sounds reasonable, you could ask the controller for which groups
> are attached to a family and then get the IDs for those groups by name.

Yes, we would need a newer api to do it right. But it could be done if
you register for multi groups.

cheers,
jamal

PS:- I just noticed Thomas wasnt CCed.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Tue, 2007-06-19 at 11:32 +0800, Zhang Rui wrote:

> > Ok, by inspection (sorry, still dont have much time) - your kernel code
> > is sending to group 1; i.e
> > 
> > genlmsg_multicast(skb, 0, 1, GFP_ATOMIC);
> > 
> > you need to change that to send to your assigned id, i.e:
> > genlmsg_multicast(skb, 0, acpi_event_genl_family.id, GFP_ATOMIC);
> > 
> Oh, that's the problem.
> Great, now it works happily. :).
> Jamal, thanks for your help!

I wonder if we should hold off on this API until we've worked out the
multicast issue.

Right now we have (mostly by convention afaict) in generic netlink that
everybody has the same group ID as the family ID but that breaks down as
soon as somebody needs more groups than that, which nl80211 will most
likely need.

Hence, the proposal Jamal had was to have a dynamic multicast number
allocator and (if I understood correctly) look up multicast numbers by
family ID/name. This is fairly extensive API/ABI change, but luckily
there are no generic netlink multicast users yet except for the
controller which luckily has the fixed ID "1".

Therefore, if we hold off on this patch until we've written the code for
dynamic multicast groups, we can hardcode the group for controller and
have all others dynamically assigned; if we merge the ACPI events now
we'll have to hardcode the ACPI family ID (and thus multicast group) to
a small number to avoid problems with dynamic multicast groups where the
numbers will be != family ID.



My proposition for the actual dynamic registration interface would be to
add a .groups array to pointers to struct genl_family with that just
being

struct genl_multicast_group {
	char *name;
	u32 id;
}
(as usual, NULL signifies array termination)

and the controller is responsible for assigning the ID and exporting it
to userspace. "name" is a per-family field, something like this patch:

---
 include/linux/genetlink.h |    3 +
 include/net/genetlink.h   |   15 ++++++
 net/netlink/genetlink.c   |  111 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)

--- wireless-dev.orig/include/net/genetlink.h	2007-06-25 23:56:59.085732308 +0200
+++ wireless-dev/include/net/genetlink.h	2007-06-26 00:01:43.935732308 +0200
@@ -5,12 +5,26 @@
 #include <net/netlink.h>
 
 /**
+ * struct genl_multicast_group - generic netlink multicast group
+ * @name: name of the multicast group, names are per-family
+ * @id: multicast group ID, assigned by the core, to use with
+ *      genlmsg_multicast().
+ */
+struct genl_multicast_group
+{
+	char	name[GENL_NAMSIZ];
+	u32	id;
+};
+
+/**
  * struct genl_family - generic netlink family
  * @id: protocol family idenfitier
  * @hdrsize: length of user specific header in bytes
  * @name: name of family
  * @version: protocol version
  * @maxattr: maximum number of attributes supported
+ * @multicast_groups: multicast groups to be registered
+ *	for this family (%NULL-terminated array)
  * @attrbuf: buffer to store parsed attributes
  * @ops_list: list of all assigned operations
  * @family_list: family list
@@ -22,6 +36,7 @@ struct genl_family
 	char			name[GENL_NAMSIZ];
 	unsigned int		version;
 	unsigned int		maxattr;
+	struct genl_multicast_group **multicast_groups;
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
--- wireless-dev.orig/net/netlink/genetlink.c	2007-06-25 23:56:02.805732308 +0200
+++ wireless-dev/net/netlink/genetlink.c	2007-06-26 00:39:26.985732308 +0200
@@ -3,6 +3,7 @@
  *
  * 		Authors:	Jamal Hadi Salim
  * 				Thomas Graf <tgraf@suug.ch>
+ *				Johannes Berg <johannes@sipsolutions.net>
  */
 
 #include <linux/module.h>
@@ -13,6 +14,7 @@
 #include <linux/string.h>
 #include <linux/skbuff.h>
 #include <linux/mutex.h>
+#include <linux/bitmap.h>
 #include <net/sock.h>
 #include <net/genetlink.h>
 
@@ -42,6 +44,15 @@ static void genl_unlock(void)
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
 static struct list_head family_ht[GENL_FAM_TAB_SIZE];
+/*
+ * To avoid an allocation at boot of just one unsigned long,
+ * declare it global instead.
+ * Bit 0 (special?) and bit 1 are marked as already used
+ * since group 1 is the controller group.
+ */
+static unsigned long mcast_group_start = 0x3;
+static unsigned long *multicast_groups = &mcast_group_start;
+static unsigned long multicast_group_bits = BITS_PER_LONG;
 
 static int genl_ctrl_event(int event, void *data);
 
@@ -116,6 +127,76 @@ static inline u16 genl_generate_id(void)
 	return id_gen_idx;
 }
 
+static int genl_register_mcast_group(struct genl_multicast_group *grp)
+{
+	int id = find_first_zero_bit(multicast_groups, multicast_group_bits);
+
+	if (id >= multicast_group_bits) {
+		if (multicast_groups == &mcast_group_start) {
+			multicast_groups =
+				kzalloc(multicast_group_bits/BITS_PER_LONG + 1,
+					GFP_KERNEL);
+			if (!multicast_groups)
+				return -ENOMEM;
+			*multicast_groups = mcast_group_start;
+		} else {
+			multicast_groups =
+				krealloc(multicast_groups,
+					 multicast_group_bits/BITS_PER_LONG + 1,
+					 GFP_KERNEL);
+			if (!multicast_groups)
+				return -ENOMEM;
+			multicast_groups[multicast_group_bits/BITS_PER_LONG] = 0;
+		}
+		multicast_group_bits += BITS_PER_LONG;
+	}
+
+	grp->id = id;
+	set_bit(id, multicast_groups);
+	return 0;
+}
+
+static void genl_unregister_mcast_group(struct genl_multicast_group *grp)
+{
+	if (grp->id) {
+		clear_bit(grp->id, multicast_groups);
+		grp->id = 0;
+	}
+}
+
+static int genl_register_mcast_groups(struct genl_family *family)
+{
+	struct genl_multicast_group **grp;
+	int err;
+
+	if (!family->multicast_groups)
+		return 0;
+
+	for (grp = family->multicast_groups; *grp; grp++) {
+		err = genl_register_mcast_group(*grp);
+		if (err) {
+			while (grp != family->multicast_groups) {
+				genl_unregister_mcast_group(*grp);
+				grp--;
+			}
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
+static void genl_unregister_mcast_groups(struct genl_family *family)
+{
+	struct genl_multicast_group **grp;
+
+	if (!family->multicast_groups)
+		return;
+
+	for (grp = family->multicast_groups; *grp; grp++)
+		genl_unregister_mcast_group(*grp);
+}
+
 /**
  * genl_register_ops - register generic netlink operations
  * @family: generic netlink family
@@ -240,6 +321,10 @@ int genl_register_family(struct genl_fam
 		family->id = newid;
 	}
 
+	err = genl_register_mcast_groups(family);
+	if (err)
+		goto errout_locked;
+
 	if (family->maxattr) {
 		family->attrbuf = kmalloc((family->maxattr+1) *
 					sizeof(struct nlattr *), GFP_KERNEL);
@@ -290,6 +375,8 @@ int genl_unregister_family(struct genl_f
 		return 0;
 	}
 
+	genl_unregister_mcast_groups(family);
+
 	genl_unlock();
 
 	return -ENOENT;
@@ -374,6 +461,7 @@ static int ctrl_fill_info(struct genl_fa
 			  u32 flags, struct sk_buff *skb, u8 cmd)
 {
 	void *hdr;
+	struct genl_multicast_group **grp;
 
 	hdr = genlmsg_put(skb, pid, seq, &genl_ctrl, flags, cmd);
 	if (hdr == NULL)
@@ -410,6 +498,29 @@ static int ctrl_fill_info(struct genl_fa
 		nla_nest_end(skb, nla_ops);
 	}
 
+	if (family->multicast_groups) {
+		struct nlattr *nla_grps;
+		int idx = 1;
+
+		nla_grps = nla_nest_start(skb, CTRL_ATTR_MCAST_GROUPS);
+		if (nla_grps == NULL)
+			goto nla_put_failure;
+
+		for (grp = family->multicast_groups; *grp; grp++) {
+			struct nlattr *nest;
+
+			nest = nla_nest_start(skb, idx++);
+			if (nest == NULL)
+				goto nla_put_failure;
+
+			NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, (*grp)->id);
+			NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME, (*grp)->name);
+
+			nla_nest_end(skb, nest);
+		}
+		nla_nest_end(skb, nla_grps);
+	}
+
 	return genlmsg_end(skb, hdr);
 
 nla_put_failure:
--- wireless-dev.orig/include/linux/genetlink.h	2007-06-25 23:56:19.895732308 +0200
+++ wireless-dev/include/linux/genetlink.h	2007-06-26 00:33:36.785732308 +0200
@@ -52,6 +52,9 @@ enum {
 	CTRL_ATTR_HDRSIZE,
 	CTRL_ATTR_MAXATTR,
 	CTRL_ATTR_OPS,
+	CTRL_ATTR_MCAST_GROUPS,
+	CTRL_ATTR_MCAST_GRP_NAME,
+	CTRL_ATTR_MCAST_GRP_ID,
 	__CTRL_ATTR_MAX,
 };
 



johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Mon, 2007-06-25 at 13:08 -0400, jamal wrote:

> > Why do you think that would be hard? It'd basically just mean replacing
> > the netlink_capable(sock, NL_NONROOT_RECV) calls with a call that
> > actually tests depending on the group(s) it wants.
> 
> I think it could be done. You will need to have root maybe initially set
> such permissions etc - but it may be overkill.

I think we pretty much know in the kernel whether we want to require
CAP_NET_ADMIN or not, let's punt the rest to userspace.

> > Yeah, sounds reasonable, you could ask the controller for which groups
> > are attached to a family and then get the IDs for those groups by name.
> 
> Yes, we would need a newer api to do it right. But it could be done if
> you register for multi groups.

I've just replied somewhere else in this thread with a patch, I haven't
actually tested that patch yet though. Once the generic netlink
multicast is figured out we can start attacking the permissions issue.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Tue, 2007-26-06 at 00:40 +0200, Johannes Berg wrote:

> I wonder if we should hold off on this API until we've worked out the
> multicast issue.

I think we can fix all the code in one shot later. I just glanced at
your patch but i have to run out, i will stare at it later - seems to be
in the right direction.

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Tue, 2007-06-26 at 09:33 -0400, jamal wrote:
> On Tue, 2007-26-06 at 00:40 +0200, Johannes Berg wrote:
> 
> > I wonder if we should hold off on this API until we've worked out the
> > multicast issue.
> 
> I think we can fix all the code in one shot later.

Yes, we could fix the code in the kernel, but since the family ID is
dynamically assigned and I'm trying to decouple the multicast group ID
from the family ID that would break userspace relying on
family==multicast group unless we somehow reserved the family ID number
ACPI got to make sure that ACPI gets the same multicast group ID.
Combined with the fact that ACPI might be modular and get into generic
netlink late in the game this seems non-trivial; also I think it's not
necessary since holding off on this ACPI genetlink multicast user (which
is the first besides the controller!) until we've worked out the patch
shouldn't hurt much.

>  I just glanced at
> your patch but i have to run out, i will stare at it later - seems to be
> in the right direction.

Thanks.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Tue, 2007-26-06 at 00:40 +0200, Johannes Berg wrote:

> I wonder if we should hold off on this API until we've worked out the
> multicast issue.

Even if the ACPI thing goes in first, you will have to change a few
others that are existing in-kernel that need to be changed sooner or
later. So either way is fine.

> My proposition for the actual dynamic registration interface would be to
> add a .groups array to pointers to struct genl_family with that just
> being
> 
> struct genl_multicast_group {
> 	char *name;
> 	u32 id;
> }

heres some feedback:
- I think you should use the same approach as we use for ops. 
a) i.e other than the reserved group for controller (which you seem to
be taking care of), every other genetlink user has to explicitly
register when they need a mcast group. 
b) While the names may vary on a per-family basis, the Grpids should be
unique (e.g when you run out of group ids - it would take a lot more
allocations than there are families; 32 bit vs 16 bit for that to
happen)
c) Use a global hash table to store all the genl_multicast_groups;
I think this (handwaving) should be searchable by i) name ii)ID and iii)
family. 

> --- wireless-dev.orig/include/net/genetlink.h	2007-06-25 23:56:59.085732308 +0200
> +++ wireless-dev/include/net/genetlink.h	2007-06-26 00:01:43.935732308 +0200
> @@ -5,12 +5,26 @@
>  #include <net/netlink.h>
>  
>  /**
> + * struct genl_multicast_group - generic netlink multicast group
> + * @name: name of the multicast group, names are per-family
> + * @id: multicast group ID, assigned by the core, to use with
> + *      genlmsg_multicast().
> + */
> +struct genl_multicast_group
> +{
> +	char	name[GENL_NAMSIZ];
> +	u32	id;
> +};

Add the list constructs on the struct - look at the way commands are
done.
We do use hash tables for global storage of families for example.
 
> +/**
>   * struct genl_family - generic netlink family
>   * @id: protocol family idenfitier
>   * @hdrsize: length of user specific header in bytes
>   * @name: name of family
>   * @version: protocol version
>   * @maxattr: maximum number of attributes supported
> + * @multicast_groups: multicast groups to be registered
> + *	for this family (%NULL-terminated array)
>   * @attrbuf: buffer to store parsed attributes
>   * @ops_list: list of all assigned operations
>   * @family_list: family list
> @@ -22,6 +36,7 @@ struct genl_family
>  	char			name[GENL_NAMSIZ];
>  	unsigned int		version;
>  	unsigned int		maxattr;
> +	struct genl_multicast_group **multicast_groups;

If you do the lists (struct list_head) you wont need this.


> +static unsigned long mcast_group_start = 0x3;
> +static unsigned long *multicast_groups = &mcast_group_start;
> +static unsigned long multicast_group_bits = BITS_PER_LONG;

I think if you used a hash table you wont need to keep track of the
above; maybe not - You may still need the above to keep track of which
IDs are in use and where holes in multicast group ID space exist
(assuming some are going to be unregistered over time etc) 


> +
> +static int genl_register_mcast_groups(struct genl_family *family)
> +{

 we use a simple scheme in the case of families; once all
IDs are consumed we dont alloc more - in the case of mcast grps this is
not necessary IMO i.e it doesnt matter if there is collision when you
run out. You return errors at the moment.


> --- wireless-dev.orig/include/linux/genetlink.h	2007-06-25 23:56:19.895732308 +0200
> +++ wireless-dev/include/linux/genetlink.h	2007-06-26 00:33:36.785732308 +0200
> @@ -52,6 +52,9 @@ enum {
>  	CTRL_ATTR_HDRSIZE,
>  	CTRL_ATTR_MAXATTR,
>  	CTRL_ATTR_OPS,
> +	CTRL_ATTR_MCAST_GROUPS,
> +	CTRL_ATTR_MCAST_GRP_NAME,
> +	CTRL_ATTR_MCAST_GRP_ID,

Dont think those last two belong in the same namespace. i.e  they should
be a separate enum; even more the name/id pair probably belong in one
TLV under the struct genl_multicast_group that is exported to user
space.

Overall: I think you are on the right path. Good stuff.
I see user space doing a discovery of which groups a family belongs to
or even doing a bind-by-name in which the underlying user-space code
does a discovery to find the id, then does a bind to that id.

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Wed, 2007-06-27 at 19:24 -0400, jamal wrote:
> On Tue, 2007-26-06 at 00:40 +0200, Johannes Berg wrote:
> 
> > I wonder if we should hold off on this API until we've worked out the
> > multicast issue.
> 
> Even if the ACPI thing goes in first, you will have to change a few
> others that are existing in-kernel that need to be changed sooner or
> later. So either way is fine.

No, the controller is the only other generic netlink multicast user
according to what I've found. :)

> heres some feedback:
> - I think you should use the same approach as we use for ops. 
> a) i.e other than the reserved group for controller (which you seem to
> be taking care of), every other genetlink user has to explicitly
> register when they need a mcast group. 

Alright. I think that most likely this will happen right at setup, but I
can change it.

> b) While the names may vary on a per-family basis, the Grpids should be
> unique (e.g when you run out of group ids - it would take a lot more
> allocations than there are families; 32 bit vs 16 bit for that to
> happen)

actually didn't you just say that groups don't run out at 32-bit because
the groups bitmap can be extended? You wouldn't be able to sign up for
the groups >31 at bind() time but with ioctls you can bind higher group
numbers after the fact. And the dynamic groups mean you need to bind
later anyway since you don't know the ID when you create the socket.

> c) Use a global hash table to store all the genl_multicast_groups;
> I think this (handwaving) should be searchable by i) name ii)ID and iii)
> family. 

Yeah, makes sense, I never liked the bitmap stuff I did there.


> Add the list constructs on the struct - look at the way commands are
> done.
> We do use hash tables for global storage of families for example.

Right, with dynamic registration that's basically a given.
 
> > +static unsigned long mcast_group_start = 0x3;
> > +static unsigned long *multicast_groups = &mcast_group_start;
> > +static unsigned long multicast_group_bits = BITS_PER_LONG;
> 
> I think if you used a hash table you wont need to keep track of the
> above; maybe not - You may still need the above to keep track of which
> IDs are in use and where holes in multicast group ID space exist
> (assuming some are going to be unregistered over time etc) 

Ah, holes are a good point. I'll think about it.

>  we use a simple scheme in the case of families; once all
> IDs are consumed we dont alloc more - in the case of mcast grps this is
> not necessary IMO i.e it doesnt matter if there is collision when you
> run out. You return errors at the moment.

Are you saying I should double-allocate groups? I really don't want that
since I plan to add permission checks on top of this.

> > --- wireless-dev.orig/include/linux/genetlink.h	2007-06-25 23:56:19.895732308 +0200
> > +++ wireless-dev/include/linux/genetlink.h	2007-06-26 00:33:36.785732308 +0200
> > @@ -52,6 +52,9 @@ enum {
> >  	CTRL_ATTR_HDRSIZE,
> >  	CTRL_ATTR_MAXATTR,
> >  	CTRL_ATTR_OPS,
> > +	CTRL_ATTR_MCAST_GROUPS,
> > +	CTRL_ATTR_MCAST_GRP_NAME,
> > +	CTRL_ATTR_MCAST_GRP_ID,
> 
> Dont think those last two belong in the same namespace. i.e  they should
> be a separate enum; even more the name/id pair probably belong in one
> TLV under the struct genl_multicast_group that is exported to user
> space.

Hm? Not sure I understand this. These are attributes for the generic
netlink controller messages, where else should I put them? Why give them
numbers from a different namespace because they're used inside nested
attributes?

> Overall: I think you are on the right path. Good stuff.
> I see user space doing a discovery of which groups a family belongs to
> or even doing a bind-by-name in which the underlying user-space code
> does a discovery to find the id, then does a bind to that id.

Yeah, that's what I was thinking of, although the bind-by-name needs
(family-id, group-name) and nost just group-name

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Thu, 2007-28-06 at 11:45 +0200, Johannes Berg wrote:

> No, the controller is the only other generic netlink multicast user
> according to what I've found. :)

Scanning the code - true ;-> Iam a little suprised that the task
accounting folks didnt use it to periodically dump stats. 

> actually didn't you just say that groups don't run out at 32-bit because
> the groups bitmap can be extended? You wouldn't be able to sign up for
> the groups >31 at bind() time but with ioctls you can bind higher group
> numbers after the fact. And the dynamic groups mean you need to bind
> later anyway since you don't know the ID when you create the socket.

Sorry i meant there are 2^16 families (-1 considering controller)
There are 2^32 groups (-1 considering controller) for the same number of
families. i.e i see those items as being global within genetlink.

> > c) Use a global hash table to store all the genl_multicast_groups;
> > I think this (handwaving) should be searchable by i) name ii)ID and iii)
> > family. 
> 
> Yeah, makes sense, I never liked the bitmap stuff I did there.
> 

It may be needed and better than how we keep track of the families - I
was just making a handwaving suggestion; when you code/test it will
become obvious.


> >  we use a simple scheme in the case of families; once all
> > IDs are consumed we dont alloc more - in the case of mcast grps this is
> > not necessary IMO i.e it doesnt matter if there is collision when you
> > run out. You return errors at the moment.
> 
> Are you saying I should double-allocate groups? 

I am not sure "double-allocate" would be the right description.
It sort of like shared-irq or a netdevice running in promisc/bcast mode.
More below.

> I really don't want that
> since I plan to add permission checks on top of this.

It is just a boundary condition policy. When there are no more groups
left (Note: it will take a lot of effort to hit that), then what do you
do?
Your current approach seems to say "return an error". My suggestion is 
to just reuse the first or last allocated one or reserve some groups for
sharing and always make sure someone is guaranteed they will get a group
when they ask for it. Infact you can even let the family tell you on
registration whether it is willing to share (and fail allocation if
nothing shareable is available).
Each family when using a shared group will always have a unique name to
itself, so discovery from user space still works - but the id will be
unique across multiple groups. User space code will be like promisc 
mode in networking, it will have to filter out the noise. If i ask the
controller to tell me about a family - it will show all group name/id
and a tag whether it is shared or not (that way my user space code knows
it needs to ignore noise it sees).
I am begining to wonder if Evgeniy's connector actually does this - i
dont remember. I am almost certain TIPC also does something similar.
In any case, this is a corner case - the suggestion is how to deal with
it. You can skin it many ways. Toss a coin - pick one.

> > > --- wireless-dev.orig/include/linux/genetlink.h	2007-06-25 23:56:19.895732308 +0200
> > > +++ wireless-dev/include/linux/genetlink.h	2007-06-26 00:33:36.785732308 +0200
> > > @@ -52,6 +52,9 @@ enum {
> > >  	CTRL_ATTR_HDRSIZE,
> > >  	CTRL_ATTR_MAXATTR,
> > >  	CTRL_ATTR_OPS,
> > > +	CTRL_ATTR_MCAST_GROUPS,
> > > +	CTRL_ATTR_MCAST_GRP_NAME,
> > > +	CTRL_ATTR_MCAST_GRP_ID,
> > 
> > Dont think those last two belong in the same namespace. i.e  they should
> > be a separate enum; even more the name/id pair probably belong in one
> > TLV under the struct genl_multicast_group that is exported to user
> > space.
> 
> Hm? Not sure I understand this. These are attributes for the generic
> netlink controller messages, 

Group name/id are nested into CTRL_ATTR_MCAST_GROUPS. Once you nest, you
are starting to a new namespace i.e as good practice you start counting
from 0. 

> where else should I put them? 

A new enum.

> Why give them
> numbers from a different namespace because they're used inside nested
> attributes?

Sorry - i should have read up to here ;-> Yes, it is because of the
nesting. Again consider the suggestion of sending just one TLV.

> > Overall: I think you are on the right path. Good stuff.
> > I see user space doing a discovery of which groups a family belongs to
> > or even doing a bind-by-name in which the underlying user-space code
> > does a discovery to find the id, then does a bind to that id.
> 
> Yeah, that's what I was thinking of, although the bind-by-name needs
> (family-id, group-name) and nost just group-name

Well, you can hide that from the user in the form of a library etc. They
dont have to know; what they know is group named "link-events" is where
they can join to listen to link events (and your abstraction generic
code does all the dirty work). 

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Fri, 2007-06-29 at 07:17 -0400, jamal wrote:

> > No, the controller is the only other generic netlink multicast user
> > according to what I've found. :)
> 
> Scanning the code - true ;-> Iam a little suprised that the task
> accounting folks didnt use it to periodically dump stats. 

:)
Because of this I'd really prefer if we could hold off on adding groups,
but I can handle both cases fine by just reserving a family and group ID
for the current users.

> Sorry i meant there are 2^16 families (-1 considering controller)
> There are 2^32 groups (-1 considering controller) for the same number of
> families. i.e i see those items as being global within genetlink.

Yeah, so we shouldn't really be worried about running out of either.

> It is just a boundary condition policy. When there are no more groups
> left (Note: it will take a lot of effort to hit that), then what do you
> do?
> Your current approach seems to say "return an error".

[suggestion snipped]

I think I'd prefer if we just returned an error. See, we aren't going to
run out of groups in the foreseeable future, and if we ever have users
that generate *a lot* of groups we can still add the sharing code etc.
For now it seems just bloat and a code path we won't ever touch so prone
to errors in it.

> > Why give them
> > numbers from a different namespace because they're used inside nested
> > attributes?
> 
> Sorry - i should have read up to here ;-> Yes, it is because of the
> nesting. Again consider the suggestion of sending just one TLV.

Ok :) Though I'm not sure I understood the suggestion of sending just
one TLV, what should I send? Or are you referring to dynamic group
registration where the whole nesting is going away anyway since you get
one message per new group...

> Well, you can hide that from the user in the form of a library etc. They
> dont have to know; what they know is group named "link-events" is where
> they can join to listen to link events (and your abstraction generic
> code does all the dirty work). 

Right. We'll see how it turns out in practice when we start using it :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-29-06 at 13:28 +0200, Johannes Berg wrote:
> On Fri, 2007-06-29 at 07:17 -0400, jamal wrote:
> 

> Because of this I'd really prefer if we could hold off on adding groups,
> but I can handle both cases fine by just reserving a family and group ID
> for the current users.
> 

sure - if you rush you can make it into Daves 2.6.23; then both can
conform at the same time.


> I think I'd prefer if we just returned an error. See, we aren't going to
> run out of groups in the foreseeable future, and if we ever have users
> that generate *a lot* of groups we can still add the sharing code etc.
> For now it seems just bloat and a code path we won't ever touch so prone
> to errors in it.
> 

Ok, you are doing the coding and i dont have strong opinions on either;
so go for it.

> Ok :) Though I'm not sure I understood the suggestion of sending just
> one TLV, what should I send? 

I meant when user space asks the controller "please tell me details
about family X" you will always pass an id and name. Never just one.
So why not just send that in a struct which has a id/name (as you
already have defined in your current patch).

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Wed, 2007-06-27 at 19:24 -0400, jamal wrote:
> 
>>c) Use a global hash table to store all the genl_multicast_groups;
>>I think this (handwaving) should be searchable by i) name ii)ID and iii)
>>family. 
> 
> 
> Yeah, makes sense, I never liked the bitmap stuff I did there.


Do multicast groups have to have a seperate name? Or would it suffice
to have them associated with the genl family and be able to find out
the starting group number? In that case something like

struct genl_mc_groups {
	struct genl_family *family or char *family_name or similar;
	unsigned int group_off;
	unsigned int group_num;
	unsigned long groups[];
};

seems to make more sense since you only need a single struct
per family.

>>>+static unsigned long mcast_group_start = 0x3;
>>>+static unsigned long *multicast_groups = &mcast_group_start;
>>>+static unsigned long multicast_group_bits = BITS_PER_LONG;


That looks pretty similar.

>>I think if you used a hash table you wont need to keep track of the
>>above; maybe not - You may still need the above to keep track of which
>>IDs are in use and where holes in multicast group ID space exist
>>(assuming some are going to be unregistered over time etc) 


Why would you care about holes? If you really want to use sparse
bitmaps that would complicate the code a lot.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Fri, 2007-06-29 at 07:48 -0400, jamal wrote:

> sure - if you rush you can make it into Daves 2.6.23; then both can
> conform at the same time.

Yeah, I'll have to see whether I can make that. If not, no big deal
either.

> > Ok :) Though I'm not sure I understood the suggestion of sending just
> > one TLV, what should I send? 
> 
> I meant when user space asks the controller "please tell me details
> about family X" you will always pass an id and name. Never just one.
> So why not just send that in a struct which has a id/name (as you
> already have defined in your current patch).

I'll have to look at the struct stuff again, but yeah, something like
that can be done.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Fri, 2007-06-29 at 13:51 +0200, Patrick McHardy wrote:

> Do multicast groups have to have a seperate name? Or would it suffice
> to have them associated with the genl family and be able to find out
> the starting group number? In that case something like
> 
> struct genl_mc_groups {
> 	struct genl_family *family or char *family_name or similar;
> 	unsigned int group_off;
> 	unsigned int group_num;
> 	unsigned long groups[];
> };
> 
> seems to make more sense since you only need a single struct
> per family.

Hm. For me that'd work too but Jamal wanted dynamically allocated groups
if I understood him correctly. I'm not too concerned with that case, I'd
think most people know the groups up-front. On the other hand, I can see
something like a group per netdev or whatever other instance too.

> Why would you care about holes? If you really want to use sparse
> bitmaps that would complicate the code a lot.

No, not sparse bitmaps, but the bitmap could have a hole when a family
goes away, and we could reuse that group number later. If we have it in
a bitmap we know without checking all group IDs.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-29-06 at 13:51 +0200, Patrick McHardy wrote:

> Do multicast groups have to have a seperate name? 

As i see it: the name would be unique per family
Its like DNS IP to name mapping essentially (in the simple case of IP
being globaly reachable). You do a discovery of the ID by knowing the
name.
 
> Or would it suffice
> to have them associated with the genl family and be able to find out
> the starting group number? 

The id space is global.

> In that case something like
> 
> struct genl_mc_groups {
> 	struct genl_family *family or char *family_name or similar;
> 	unsigned int group_off;
> 	unsigned int group_num;
> 	unsigned long groups[];
> };
> 
> seems to make more sense since you only need a single struct
> per family.

I think something that ties to the family would be needed.

> >>>+static unsigned long mcast_group_start = 0x3;
> >>>+static unsigned long *multicast_groups = &mcast_group_start;
> >>>+static unsigned long multicast_group_bits = BITS_PER_LONG;
> 
> 
> That looks pretty similar.

I know-;> when i first saw it i asked myself "Hrm, where have i seen
that before?" ;->
 
> Why would you care about holes? If you really want to use sparse
> bitmaps that would complicate the code a lot.

similar to ifindices. You want to reuse/recycle them. 

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Fri, 2007-06-29 at 13:51 +0200, Patrick McHardy wrote:
> 
> 
>>Do multicast groups have to have a seperate name? Or would it suffice
>>to have them associated with the genl family and be able to find out
>>the starting group number? In that case something like
>>
>>struct genl_mc_groups {
>>	struct genl_family *family or char *family_name or similar;
>>	unsigned int group_off;
>>	unsigned int group_num;
>>	unsigned long groups[];
>>};
>>
>>seems to make more sense since you only need a single struct
>>per family.
> 
> 
> Hm. For me that'd work too but Jamal wanted dynamically allocated groups
> if I understood him correctly. I'm not too concerned with that case, I'd
> think most people know the groups up-front. On the other hand, I can see
> something like a group per netdev or whatever other instance too.


Maybe use a mix. Use the bitmap, but allow families to register
multiple of them. In the common case it would only be a single
one, but it would be possible to register groups dynamically.

>>Why would you care about holes? If you really want to use sparse
>>bitmaps that would complicate the code a lot.
> 
> 
> No, not sparse bitmaps, but the bitmap could have a hole when a family
> goes away, and we could reuse that group number later. If we have it in
> a bitmap we know without checking all group IDs.


Right.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

jamal wrote:
> On Fri, 2007-29-06 at 13:51 +0200, Patrick McHardy wrote:
> 
> 
>>Do multicast groups have to have a seperate name? 
> 
> 
> As i see it: the name would be unique per family
> Its like DNS IP to name mapping essentially (in the simple case of IP
> being globaly reachable). You do a discovery of the ID by knowing the
> name.


Mhh .. I agree that it would be necessary to have a group identifier
for dynamically allocated groups like one group per device or something.
I'm wondering if anyone really needs this though. Having messages
grouped logically by type makes more sense to me.

>>Or would it suffice
>>to have them associated with the genl family and be able to find out
>>the starting group number? 
> 
> 
> The id space is global.
> 
> 
>>In that case something like
>>
>>struct genl_mc_groups {
>>	struct genl_family *family or char *family_name or similar;
>>	unsigned int group_off;
>>	unsigned int group_num;
>>	unsigned long groups[];
>>};
>>
>>seems to make more sense since you only need a single struct
>>per family.
> 
> 
> I think something that ties to the family would be needed.


My example includes a pointer to the family or the name, but anything
else that works is fine also.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Fri, 2007-06-29 at 13:51 +0200, Patrick McHardy wrote:

> Do multicast groups have to have a seperate name? Or would it suffice
> to have them associated with the genl family and be able to find out
> the starting group number? In that case something like
> 
> struct genl_mc_groups {
> 	struct genl_family *family or char *family_name or similar;
> 	unsigned int group_off;
> 	unsigned int group_num;
> 	unsigned long groups[];
> };
> 
> seems to make more sense since you only need a single struct
> per family.

Hmm, another thought: since we have 32 bits for group numbers and 16
bits for families we could just reserve 16 bits for groups within each
family. Or do we get trouble with that because in some place there's a
group bitmap or such?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Fri, 2007-06-29 at 13:51 +0200, Patrick McHardy wrote:
> 
> 
>>Do multicast groups have to have a seperate name? Or would it suffice
>>to have them associated with the genl family and be able to find out
>>the starting group number? In that case something like
>>
>>struct genl_mc_groups {
>>	struct genl_family *family or char *family_name or similar;
>>	unsigned int group_off;
>>	unsigned int group_num;
>>	unsigned long groups[];
>>};
>>
>>seems to make more sense since you only need a single struct
>>per family.
> 
> 
> Hmm, another thought: since we have 32 bits for group numbers and 16
> bits for families we could just reserve 16 bits for groups within each
> family. Or do we get trouble with that because in some place there's a
> group bitmap or such?


Yes, af_netlink has a bitmap per socket that is subscribed to any group.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Fri, 2007-06-29 at 14:48 +0200, Patrick McHardy wrote:

> > Hmm, another thought: since we have 32 bits for group numbers and 16
> > bits for families we could just reserve 16 bits for groups within each
> > family. Or do we get trouble with that because in some place there's a
> > group bitmap or such?
> 
> 
> Yes, af_netlink has a bitmap per socket that is subscribed to any group.

Not a good idea then. I'll see what I can do.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Wed, 2007-06-27 at 19:24 -0400, jamal wrote:

> a) i.e other than the reserved group for controller (which you seem to
> be taking care of), every other genetlink user has to explicitly
> register when they need a mcast group. 

Hm. I'm starting to dislike the dynamic registration the more I think
about it. Now when a group is unregistered I'd have to unbind everybody
who's currently using it... At least when I want to enforce
root/non-root binds which is a further goal.

> c) Use a global hash table to store all the genl_multicast_groups;
> I think this (handwaving) should be searchable by i) name ii)ID and iii)
> family. 

Also, it doesn't look like I'll ever be searching for a group unless we
want to allow userspace to explicitly ask for a group ID, but I'd think
it should just get the list of all IDs when asking for family
information and then cache that.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-29-06 at 14:48 +0200, Patrick McHardy wrote:
> Johannes Berg wrote:

> > Hmm, another thought: since we have 32 bits for group numbers and 16
> > bits for families we could just reserve 16 bits for groups within each
> > family. Or do we get trouble with that because in some place there's a
> > group bitmap or such?
> 
> 
> Yes, af_netlink has a bitmap per socket that is subscribed to any group.


I think this is the challenge. The groups belong to a global namespace.
i.e when you do a socket bind to group - it is unique regardless of the
family.
Our philosophy in genetlink is to have dynamic resources allocated and
released - remember the real reason we even have this is because we were
running out of numbers ;->
So while the static allocation of 16 bits per group will work (famous
last words "noone will ever need more than 640K of RAM";->) it will be
cleaner imo to allow dynamic allocation/release.
Maybe a mix (of a few static and mostly dynamic) as Patrick says - but
that would mean more coding for you ;-> Actually i like the idea of at
least your ID being your static mcast group and the rest are in the
dynamic pool (Hey, thanks Patrick;->). This means the first 2^16 are
static/reserved and if you want more groups, you register for them.

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Wed, 2007-06-27 at 19:24 -0400, jamal wrote:
> 
> 
>>a) i.e other than the reserved group for controller (which you seem to
>>be taking care of), every other genetlink user has to explicitly
>>register when they need a mcast group. 
> 
> 
> Hm. I'm starting to dislike the dynamic registration the more I think
> about it. Now when a group is unregistered I'd have to unbind everybody
> who's currently using it... At least when I want to enforce
> root/non-root binds which is a further goal.


How about using module references to prevent unloading while sockets
are bound? We already do the same thing for netlink families.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-29-06 at 14:57 +0200, Johannes Berg wrote:
> On Wed, 2007-06-27 at 19:24 -0400, jamal wrote:

> Hm. I'm starting to dislike the dynamic registration the more I think
> about it. Now when a group is unregistered I'd have to unbind everybody
> who's currently using it... 

I understood you earlier to say only one family (in kernel) can own a
group else it is an error, no? So there is "somebody" but not
"everybody"
BTW, refer to my earlier email in response to Patrick. I think
preserving a group per family is also an interesting idea. i.e
the first 2^16 are reserved/static and the others are in all for
grabs area.

> At least when I want to enforce
> root/non-root binds which is a further goal.

Permissions would be group attributes - maybe something in the group
struct to say who is allowed to bind?

> > c) Use a global hash table to store all the genl_multicast_groups;
> > I think this (handwaving) should be searchable by i) name ii)ID and iii)
> > family. 
> 
> Also, it doesn't look like I'll ever be searching for a group unless we
> want to allow userspace to explicitly ask for a group ID, but I'd think
> it should just get the list of all IDs when asking for family
> information and then cache that.

I think its useful to have the filters in the kernel when
possible/sensible (one of my beefs against netlink in general is it
lacks that).

cheers,
jamal


> johannes

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

jamal wrote:
> On Fri, 2007-29-06 at 14:48 +0200, Patrick McHardy wrote:
> 
>>Johannes Berg wrote:
> 
> 
>>>Hmm, another thought: since we have 32 bits for group numbers and 16
>>>bits for families we could just reserve 16 bits for groups within each
>>>family. Or do we get trouble with that because in some place there's a
>>>group bitmap or such?
>>
>>
>>Yes, af_netlink has a bitmap per socket that is subscribed to any group.
> 
> 
> 
> I think this is the challenge. The groups belong to a global namespace.
> i.e when you do a socket bind to group - it is unique regardless of the
> family.
> Our philosophy in genetlink is to have dynamic resources allocated and
> released - remember the real reason we even have this is because we were
> running out of numbers ;->


That was more of a rumour :) We have 2^32-1 groups and I think 256
families, of which about 20 are used.

> So while the static allocation of 16 bits per group will work (famous
> last words "noone will ever need more than 640K of RAM";->) it will be
> cleaner imo to allow dynamic allocation/release.
> Maybe a mix (of a few static and mostly dynamic) as Patrick says - but
> that would mean more coding for you ;-> Actually i like the idea of at
> least your ID being your static mcast group and the rest are in the
> dynamic pool (Hey, thanks Patrick;->). This means the first 2^16 are
> static/reserved and if you want more groups, you register for them.


I wouldn't reserve any, just hand them out as needed. Otherwise we'll
run into problems with the af_netlink bitmaps.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Fri, 2007-06-29 at 09:02 -0400, jamal wrote:

> Maybe a mix (of a few static and mostly dynamic) as Patrick says - but
> that would mean more coding for you ;-> Actually i like the idea of at
> least your ID being your static mcast group and the rest are in the
> dynamic pool (Hey, thanks Patrick;->). This means the first 2^16 are
> static/reserved and if you want more groups, you register for them.

Hah, that means I get to rewrite af_netlink.c to not have a bitmap since
that'd mean that with just a single dynamically registered multicast
group it gets ID 2^16 which means an 8 kilobyte bitmap...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Fri, 2007-06-29 at 15:11 +0200, Patrick McHardy wrote:

> > Hm. I'm starting to dislike the dynamic registration the more I think
> > about it. Now when a group is unregistered I'd have to unbind everybody
> > who's currently using it... At least when I want to enforce
> > root/non-root binds which is a further goal.
> 
> 
> How about using module references to prevent unloading while sockets
> are bound? We already do the same thing for netlink families.

Ah, no, you're both thinking something different. I'm thinking if we
allow dynamic registration the only sensible thing is to have dynamic
unregistration as well, and then we can have this sequence

 (1) register group X with non-root
 (2) non-root app A binds group X
 (3) kernel unregisters group X
 (4) something else in kernel reregisters group ID X but root-only
-> non-root app A is bound to root-only group X

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Fri, 2007-06-29 at 15:11 +0200, Patrick McHardy wrote:
> 
> 
>>>Hm. I'm starting to dislike the dynamic registration the more I think
>>>about it. Now when a group is unregistered I'd have to unbind everybody
>>>who's currently using it... At least when I want to enforce
>>>root/non-root binds which is a further goal.
>>
>>
>>How about using module references to prevent unloading while sockets
>>are bound? We already do the same thing for netlink families.
> 
> 
> Ah, no, you're both thinking something different. I'm thinking if we
> allow dynamic registration the only sensible thing is to have dynamic
> unregistration as well, and then we can have this sequence
> 
>  (1) register group X with non-root
>  (2) non-root app A binds group X
>  (3) kernel unregisters group X
>  (4) something else in kernel reregisters group ID X but root-only
> -> non-root app A is bound to root-only group X


I'm not sure that "the only sensible thing to do" is right, we
do allow dynamic registration of netlink families and do the
module reference thing anyway (admittedly, I never liked that
and the autoloading part very much). I guess it depends on how
this will be used in the end, if you really do have a group per
device or something like that you probably need to be able to
unregister at any time. But as I said previously I believe its
more in the spirit of netlink to group things logically by
message type, in which case some core part would own the
family and not a single device.

If you do want the dynamic unregistation *and* the non-root mc
listening then I guess you don't have a choice but to unbind
sockets at unregistration. That shouln't be a real problem,
without having though much about it, I believe just clearing
the mc group from the bitmap and calling netlink_update_subscriptions
should be fine.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-29-06 at 15:15 +0200, Johannes Berg wrote:

>  (1) register group X with non-root
>  (2) non-root app A binds group X
>  (3) kernel unregisters group X
Kernel sends event (controller) "Group X is gone" or "family Y which
used to own group X is gone"

>  (4) something else in kernel reregisters group ID X but root-only
> -> non-root app A is bound to root-only group X

Kernel (controller genetlink) sends event for that too.

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-29-06 at 15:12 +0200, Patrick McHardy wrote:
> jamal wrote:
> > On Fri, 2007-29-06 at 14:48 +0200, Patrick McHardy wrote:

> > Our philosophy in genetlink is to have dynamic resources allocated and
> > released - remember the real reason we even have this is because we were
> > running out of numbers ;->
> 
> 
> That was more of a rumour :) We have 2^32-1 groups and I think 256
> families, of which about 20 are used.

Well, if thats not the case - we need to fix it ;->

Anyways, I have to run off.

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

jamal wrote:
> On Fri, 2007-29-06 at 15:12 +0200, Patrick McHardy wrote:
> 
>>jamal wrote:
>>
>>>On Fri, 2007-29-06 at 14:48 +0200, Patrick McHardy wrote:
> 
> 
>>>Our philosophy in genetlink is to have dynamic resources allocated and
>>>released - remember the real reason we even have this is because we were
>>>running out of numbers ;->
>>
>>
>>That was more of a rumour :) We have 2^32-1 groups and I think 256
>>families, of which about 20 are used.
> 
> 
> Well, if thats not the case - we need to fix it ;->


I meant the "running out of numbers" part, we have plenty left.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Fri, 2007-06-29 at 15:23 +0200, Patrick McHardy wrote:

> I'm not sure that "the only sensible thing to do" is right, we
> do allow dynamic registration of netlink families and do the
> module reference thing anyway (admittedly, I never liked that
> and the autoloading part very much). I guess it depends on how
> this will be used in the end, if you really do have a group per
> device or something like that you probably need to be able to
> unregister at any time. But as I said previously I believe its
> more in the spirit of netlink to group things logically by
> message type, in which case some core part would own the
> family and not a single device.
> 
> If you do want the dynamic unregistation *and* the non-root mc
> listening then I guess you don't have a choice but to unbind
> sockets at unregistration. That shouln't be a real problem,
> without having though much about it, I believe just clearing
> the mc group from the bitmap and calling netlink_update_subscriptions
> should be fine.

Yeah, true, but the patch gets larger with every little thing here :)

How about for now I only allow dynamic registration (no unregistration)
and just send out when new groups are registered, and also give
userspace a list of registered mc groups when they ask for a family
description? That should make this patch not too big and still leaves
room for dynamic unregistration later.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Fri, 2007-06-29 at 15:23 +0200, Patrick McHardy wrote:
> 
>>If you do want the dynamic unregistation *and* the non-root mc
>>listening then I guess you don't have a choice but to unbind
>>sockets at unregistration. That shouln't be a real problem,
>>without having though much about it, I believe just clearing
>>the mc group from the bitmap and calling netlink_update_subscriptions
>>should be fine.
> 
> 
> Yeah, true, but the patch gets larger with every little thing here :)


Just do a seperate patch to add mc unregistation to af_netlink.

> How about for now I only allow dynamic registration (no unregistration)
> and just send out when new groups are registered, and also give
> userspace a list of registered mc groups when they ask for a family
> description? That should make this patch not too big and still leaves
> room for dynamic unregistration later.


How does it deal with unregistration currently? If it leaves sockets
subscribed that seems like a bug already, at least if the id was
dynamically generated. Of course it would require the id to be
reused to actually matter.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Fri, 2007-06-29 at 15:44 +0200, Patrick McHardy wrote:

> > How about for now I only allow dynamic registration (no unregistration)
> > and just send out when new groups are registered, and also give
> > userspace a list of registered mc groups when they ask for a family
> > description? That should make this patch not too big and still leaves
> > room for dynamic unregistration later.
> 
> 
> How does it deal with unregistration currently? If it leaves sockets
> subscribed that seems like a bug already, at least if the id was
> dynamically generated. Of course it would require the id to be
> reused to actually matter.

Hmm. I don't see it kicking out the socket subscriptions in
genl_unregister_family so I guess that bug is present.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Fri, 2007-06-29 at 15:44 +0200, Patrick McHardy wrote:
> 
> 
>>>How about for now I only allow dynamic registration (no unregistration)
>>>and just send out when new groups are registered, and also give
>>>userspace a list of registered mc groups when they ask for a family
>>>description? That should make this patch not too big and still leaves
>>>room for dynamic unregistration later.
>>
>>
>>How does it deal with unregistration currently? If it leaves sockets
>>subscribed that seems like a bug already, at least if the id was
>>dynamically generated. Of course it would require the id to be
>>reused to actually matter.
> 
> 
> Hmm. I don't see it kicking out the socket subscriptions in
> genl_unregister_family so I guess that bug is present.


If you're worried about patch size, you could sell that part as a
bugfix :)

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

On Fri, 2007-06-29 at 15:53 +0200, Patrick McHardy wrote:

> If you're worried about patch size, you could sell that part as a
> bugfix :)

Heh. Actually, right now I'm more worried about how much work I have to
do short-term.

This patch keeps the bitmap but does dynamic group allocation. Just to
see where I'm right now, I have a few things I'd like to change like not
sending out the full family info if it registers a new group.


---
 include/linux/genetlink.h |   13 ++++++
 include/net/genetlink.h   |   18 +++++++++
 net/netlink/genetlink.c   |   89 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+)

--- wireless-dev.orig/include/net/genetlink.h	2007-06-26 16:58:42.154806179 +0200
+++ wireless-dev/include/net/genetlink.h	2007-06-29 15:40:35.547910932 +0200
@@ -5,6 +5,20 @@
 #include <net/netlink.h>
 
 /**
+ * struct genl_multicast_group - generic netlink multicast group
+ * @name: name of the multicast group, names are per-family
+ * @id: multicast group ID, assigned by the core, to use with
+ *      genlmsg_multicast().
+ * @list: list entry for linking
+ */
+struct genl_multicast_group
+{
+	struct list_head	list;		/* private */
+	char			name[GENL_NAMSIZ];
+	u32			id;
+};
+
+/**
  * struct genl_family - generic netlink family
  * @id: protocol family idenfitier
  * @hdrsize: length of user specific header in bytes
@@ -14,6 +28,7 @@
  * @attrbuf: buffer to store parsed attributes
  * @ops_list: list of all assigned operations
  * @family_list: family list
+ * @mcast_groups: multicast groups list
  */
 struct genl_family
 {
@@ -25,6 +40,7 @@ struct genl_family
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
+	struct list_head	mcast_groups;	/* private */
 };
 
 /**
@@ -73,6 +89,8 @@ extern int genl_register_family(struct g
 extern int genl_unregister_family(struct genl_family *family);
 extern int genl_register_ops(struct genl_family *, struct genl_ops *ops);
 extern int genl_unregister_ops(struct genl_family *, struct genl_ops *ops);
+extern int genl_register_mc_group(struct genl_family *family,
+				  struct genl_multicast_group *grp);
 
 extern struct sock *genl_sock;
 
--- wireless-dev.orig/net/netlink/genetlink.c	2007-06-26 16:58:42.244806179 +0200
+++ wireless-dev/net/netlink/genetlink.c	2007-06-29 16:01:38.487910932 +0200
@@ -3,6 +3,7 @@
  *
  * 		Authors:	Jamal Hadi Salim
  * 				Thomas Graf <tgraf@suug.ch>
+ *				Johannes Berg <johannes@sipsolutions.net>
  */
 
 #include <linux/module.h>
@@ -13,6 +14,7 @@
 #include <linux/string.h>
 #include <linux/skbuff.h>
 #include <linux/mutex.h>
+#include <linux/bitmap.h>
 #include <net/sock.h>
 #include <net/genetlink.h>
 
@@ -42,6 +44,16 @@ static void genl_unlock(void)
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
 static struct list_head family_ht[GENL_FAM_TAB_SIZE];
+/*
+ * Bitmap of multicast groups that are currently in use.
+ *
+ * To avoid an allocation at boot of just one unsigned long,
+ * declare it global instead.
+ * Bit 1 is marked as already used since group 1 is the controller group.
+ */
+static unsigned long mc_group_start = 0x2;
+static unsigned long *mc_groups = &mc_group_start;
+static unsigned long mc_groups_bits = BITS_PER_LONG;
 
 static int genl_ctrl_event(int event, void *data);
 
@@ -116,6 +128,55 @@ static inline u16 genl_generate_id(void)
 	return id_gen_idx;
 }
 
+int genl_register_mc_group(struct genl_family *family,
+			   struct genl_multicast_group *grp)
+{
+	int id = find_first_zero_bit(mc_groups, mc_groups_bits);
+
+	if (id >= mc_groups_bits) {
+		if (mc_groups == &mc_group_start) {
+			mc_groups =
+				kzalloc(mc_groups_bits/BITS_PER_LONG + 1,
+					GFP_KERNEL);
+			if (!mc_groups)
+				return -ENOMEM;
+			*mc_groups = mc_group_start;
+		} else {
+			mc_groups =
+				krealloc(mc_groups,
+					 mc_groups_bits/BITS_PER_LONG + 1,
+					 GFP_KERNEL);
+			if (!mc_groups)
+				return -ENOMEM;
+			mc_groups[mc_groups_bits/BITS_PER_LONG] = 0;
+		}
+		mc_groups_bits += BITS_PER_LONG;
+	}
+
+	grp->id = id;
+	set_bit(id, mc_groups);
+	list_add_tail(&grp->list, &family->mcast_groups);
+
+	genl_ctrl_event(CTRL_CMD_NEWMCAST_GRP, family);
+
+	return 0;
+}
+EXPORT_SYMBOL(genl_register_mc_group);
+
+static void genl_unregister_mcast_group(struct genl_multicast_group *grp)
+{
+	clear_bit(grp->id, mc_groups);
+	list_del(&grp->list);
+}
+
+static void genl_unregister_mcast_groups(struct genl_family *family)
+{
+	struct genl_multicast_group *grp, *tmp;
+
+	list_for_each_entry_safe(grp, tmp, &family->mcast_groups, list)
+		genl_unregister_mcast_group(grp);
+}
+
 /**
  * genl_register_ops - register generic netlink operations
  * @family: generic netlink family
@@ -290,6 +351,8 @@ int genl_unregister_family(struct genl_f
 		return 0;
 	}
 
+	genl_unregister_mcast_groups(family);
+
 	genl_unlock();
 
 	return -ENOENT;
@@ -410,6 +473,31 @@ static int ctrl_fill_info(struct genl_fa
 		nla_nest_end(skb, nla_ops);
 	}
 
+	if (!list_empty(&family->mcast_groups)) {
+		struct genl_multicast_group *grp;
+		struct nlattr *nla_grps;
+		int idx = 1;
+
+		nla_grps = nla_nest_start(skb, CTRL_ATTR_MCAST_GROUPS);
+		if (nla_grps == NULL)
+			goto nla_put_failure;
+
+		list_for_each_entry(grp, &family->mcast_groups, list) {
+			struct nlattr *nest;
+
+			nest = nla_nest_start(skb, idx++);
+			if (nest == NULL)
+				goto nla_put_failure;
+
+			NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, grp->id);
+			NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME,
+				       grp->name);
+
+			nla_nest_end(skb, nest);
+		}
+		nla_nest_end(skb, nla_grps);
+	}
+
 	return genlmsg_end(skb, hdr);
 
 nla_put_failure:
@@ -523,6 +611,7 @@ static int genl_ctrl_event(int event, vo
 	switch (event) {
 	case CTRL_CMD_NEWFAMILY:
 	case CTRL_CMD_DELFAMILY:
+	case CTRL_CMD_NEWMCAST_GRP:
 		msg = ctrl_build_msg(data, 0, 0, event);
 		if (IS_ERR(msg))
 			return PTR_ERR(msg);
--- wireless-dev.orig/include/linux/genetlink.h	2007-06-26 16:58:42.174806179 +0200
+++ wireless-dev/include/linux/genetlink.h	2007-06-29 16:00:05.687910932 +0200
@@ -39,6 +39,9 @@ enum {
 	CTRL_CMD_NEWOPS,
 	CTRL_CMD_DELOPS,
 	CTRL_CMD_GETOPS,
+	CTRL_CMD_NEWMCAST_GRP,
+	CTRL_CMD_DELMCAST_GRP, /* unused */
+	CTRL_CMD_GETMCAST_GRP, /* unused */
 	__CTRL_CMD_MAX,
 };
 
@@ -52,6 +55,7 @@ enum {
 	CTRL_ATTR_HDRSIZE,
 	CTRL_ATTR_MAXATTR,
 	CTRL_ATTR_OPS,
+	CTRL_ATTR_MCAST_GROUPS,
 	__CTRL_ATTR_MAX,
 };
 
@@ -66,4 +70,13 @@ enum {
 
 #define CTRL_ATTR_OP_MAX (__CTRL_ATTR_OP_MAX - 1)
 
+enum {
+	CTRL_ATTR_MCAST_GRP_UNSPEC,
+	CTRL_ATTR_MCAST_GRP_NAME,
+	CTRL_ATTR_MCAST_GRP_ID,
+	__CTRL_ATTR_MCAST_GRP_MAX,
+};
+
+#define CTRL_ATTR_MCAST_GRP_MAX (__CTRL_ATTR_MCAST_GRP_MAX - 1)
+
 #endif	/* __LINUX_GENERIC_NETLINK_H */

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

On Fri, 2007-06-29 at 16:05 +0200, Johannes Berg wrote:

> +			mc_groups =
> +				kzalloc(mc_groups_bits/BITS_PER_LONG + 1,
> +					GFP_KERNEL);
> +			if (!mc_groups)
> +				return -ENOMEM;
> +			*mc_groups = mc_group_start;

Uh huh, this reallocation is buggy. Fixed version below.

---
 include/linux/genetlink.h |   13 ++++++
 include/net/genetlink.h   |   18 +++++++++
 net/netlink/genetlink.c   |   90 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+)

--- wireless-dev.orig/include/net/genetlink.h	2007-06-26 16:58:42.154806179 +0200
+++ wireless-dev/include/net/genetlink.h	2007-06-29 15:40:35.547910932 +0200
@@ -5,6 +5,20 @@
 #include <net/netlink.h>
 
 /**
+ * struct genl_multicast_group - generic netlink multicast group
+ * @name: name of the multicast group, names are per-family
+ * @id: multicast group ID, assigned by the core, to use with
+ *      genlmsg_multicast().
+ * @list: list entry for linking
+ */
+struct genl_multicast_group
+{
+	struct list_head	list;		/* private */
+	char			name[GENL_NAMSIZ];
+	u32			id;
+};
+
+/**
  * struct genl_family - generic netlink family
  * @id: protocol family idenfitier
  * @hdrsize: length of user specific header in bytes
@@ -14,6 +28,7 @@
  * @attrbuf: buffer to store parsed attributes
  * @ops_list: list of all assigned operations
  * @family_list: family list
+ * @mcast_groups: multicast groups list
  */
 struct genl_family
 {
@@ -25,6 +40,7 @@ struct genl_family
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
+	struct list_head	mcast_groups;	/* private */
 };
 
 /**
@@ -73,6 +89,8 @@ extern int genl_register_family(struct g
 extern int genl_unregister_family(struct genl_family *family);
 extern int genl_register_ops(struct genl_family *, struct genl_ops *ops);
 extern int genl_unregister_ops(struct genl_family *, struct genl_ops *ops);
+extern int genl_register_mc_group(struct genl_family *family,
+				  struct genl_multicast_group *grp);
 
 extern struct sock *genl_sock;
 
--- wireless-dev.orig/net/netlink/genetlink.c	2007-06-26 16:58:42.244806179 +0200
+++ wireless-dev/net/netlink/genetlink.c	2007-06-29 16:14:06.307910932 +0200
@@ -3,6 +3,7 @@
  *
  * 		Authors:	Jamal Hadi Salim
  * 				Thomas Graf <tgraf@suug.ch>
+ *				Johannes Berg <johannes@sipsolutions.net>
  */
 
 #include <linux/module.h>
@@ -13,6 +14,7 @@
 #include <linux/string.h>
 #include <linux/skbuff.h>
 #include <linux/mutex.h>
+#include <linux/bitmap.h>
 #include <net/sock.h>
 #include <net/genetlink.h>
 
@@ -42,6 +44,16 @@ static void genl_unlock(void)
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
 static struct list_head family_ht[GENL_FAM_TAB_SIZE];
+/*
+ * Bitmap of multicast groups that are currently in use.
+ *
+ * To avoid an allocation at boot of just one unsigned long,
+ * declare it global instead.
+ * Bit 1 is marked as already used since group 1 is the controller group.
+ */
+static unsigned long mc_group_start = 0x2;
+static unsigned long *mc_groups = &mc_group_start;
+static unsigned long mc_groups_bits = BITS_PER_LONG;
 
 static int genl_ctrl_event(int event, void *data);
 
@@ -116,6 +128,56 @@ static inline u16 genl_generate_id(void)
 	return id_gen_idx;
 }
 
+int genl_register_mc_group(struct genl_family *family,
+			   struct genl_multicast_group *grp)
+{
+	int id = find_first_zero_bit(mc_groups, mc_groups_bits);
+	unsigned long *new_groups;
+
+	if (id >= mc_groups_bits) {
+		if (mc_groups == &mc_group_start) {
+			new_groups = kzalloc(mc_groups_bits/BITS_PER_LONG + 1,
+					     GFP_KERNEL);
+			if (!mc_groups)
+				return -ENOMEM;
+			mc_groups = new_groups;
+			*mc_groups = mc_group_start;
+		} else {
+			new_groups = krealloc(mc_groups,
+					      mc_groups_bits/BITS_PER_LONG + 1,
+					      GFP_KERNEL);
+			if (!new_groups)
+				return -ENOMEM;
+			mc_groups = new_groups;
+			mc_groups[mc_groups_bits/BITS_PER_LONG] = 0;
+		}
+		mc_groups_bits += BITS_PER_LONG;
+	}
+
+	grp->id = id;
+	set_bit(id, mc_groups);
+	list_add_tail(&grp->list, &family->mcast_groups);
+
+	genl_ctrl_event(CTRL_CMD_NEWMCAST_GRP, family);
+
+	return 0;
+}
+EXPORT_SYMBOL(genl_register_mc_group);
+
+static void genl_unregister_mcast_group(struct genl_multicast_group *grp)
+{
+	clear_bit(grp->id, mc_groups);
+	list_del(&grp->list);
+}
+
+static void genl_unregister_mcast_groups(struct genl_family *family)
+{
+	struct genl_multicast_group *grp, *tmp;
+
+	list_for_each_entry_safe(grp, tmp, &family->mcast_groups, list)
+		genl_unregister_mcast_group(grp);
+}
+
 /**
  * genl_register_ops - register generic netlink operations
  * @family: generic netlink family
@@ -290,6 +352,8 @@ int genl_unregister_family(struct genl_f
 		return 0;
 	}
 
+	genl_unregister_mcast_groups(family);
+
 	genl_unlock();
 
 	return -ENOENT;
@@ -410,6 +474,31 @@ static int ctrl_fill_info(struct genl_fa
 		nla_nest_end(skb, nla_ops);
 	}
 
+	if (!list_empty(&family->mcast_groups)) {
+		struct genl_multicast_group *grp;
+		struct nlattr *nla_grps;
+		int idx = 1;
+
+		nla_grps = nla_nest_start(skb, CTRL_ATTR_MCAST_GROUPS);
+		if (nla_grps == NULL)
+			goto nla_put_failure;
+
+		list_for_each_entry(grp, &family->mcast_groups, list) {
+			struct nlattr *nest;
+
+			nest = nla_nest_start(skb, idx++);
+			if (nest == NULL)
+				goto nla_put_failure;
+
+			NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, grp->id);
+			NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME,
+				       grp->name);
+
+			nla_nest_end(skb, nest);
+		}
+		nla_nest_end(skb, nla_grps);
+	}
+
 	return genlmsg_end(skb, hdr);
 
 nla_put_failure:
@@ -523,6 +612,7 @@ static int genl_ctrl_event(int event, vo
 	switch (event) {
 	case CTRL_CMD_NEWFAMILY:
 	case CTRL_CMD_DELFAMILY:
+	case CTRL_CMD_NEWMCAST_GRP:
 		msg = ctrl_build_msg(data, 0, 0, event);
 		if (IS_ERR(msg))
 			return PTR_ERR(msg);
--- wireless-dev.orig/include/linux/genetlink.h	2007-06-26 16:58:42.174806179 +0200
+++ wireless-dev/include/linux/genetlink.h	2007-06-29 16:00:05.687910932 +0200
@@ -39,6 +39,9 @@ enum {
 	CTRL_CMD_NEWOPS,
 	CTRL_CMD_DELOPS,
 	CTRL_CMD_GETOPS,
+	CTRL_CMD_NEWMCAST_GRP,
+	CTRL_CMD_DELMCAST_GRP, /* unused */
+	CTRL_CMD_GETMCAST_GRP, /* unused */
 	__CTRL_CMD_MAX,
 };
 
@@ -52,6 +55,7 @@ enum {
 	CTRL_ATTR_HDRSIZE,
 	CTRL_ATTR_MAXATTR,
 	CTRL_ATTR_OPS,
+	CTRL_ATTR_MCAST_GROUPS,
 	__CTRL_ATTR_MAX,
 };
 
@@ -66,4 +70,13 @@ enum {
 
 #define CTRL_ATTR_OP_MAX (__CTRL_ATTR_OP_MAX - 1)
 
+enum {
+	CTRL_ATTR_MCAST_GRP_UNSPEC,
+	CTRL_ATTR_MCAST_GRP_NAME,
+	CTRL_ATTR_MCAST_GRP_ID,
+	__CTRL_ATTR_MCAST_GRP_MAX,
+};
+
+#define CTRL_ATTR_MCAST_GRP_MAX (__CTRL_ATTR_MCAST_GRP_MAX - 1)
+
 #endif	/* __LINUX_GENERIC_NETLINK_H */

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

On Fri, 2007-06-29 at 16:19 +0200, Johannes Berg wrote:

> Uh huh, this reallocation is buggy. Fixed version below.

More breakage, sorry about the patch-spam.

Btw, I notice that the bug we talked about isn't present in practice
since we have no multicast users except for the always-present
controller :)

New patch below. I'll get this tested and maybe a bit nicer and I'll try
to fix the bug too, but not before the weekend.

---
 include/linux/genetlink.h |   13 ++++++
 include/net/genetlink.h   |   18 ++++++++
 net/netlink/genetlink.c   |   93 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)

--- wireless-dev.orig/include/net/genetlink.h	2007-06-26 16:58:42.154806179 +0200
+++ wireless-dev/include/net/genetlink.h	2007-06-29 15:40:35.547910932 +0200
@@ -5,6 +5,20 @@
 #include <net/netlink.h>
 
 /**
+ * struct genl_multicast_group - generic netlink multicast group
+ * @name: name of the multicast group, names are per-family
+ * @id: multicast group ID, assigned by the core, to use with
+ *      genlmsg_multicast().
+ * @list: list entry for linking
+ */
+struct genl_multicast_group
+{
+	struct list_head	list;		/* private */
+	char			name[GENL_NAMSIZ];
+	u32			id;
+};
+
+/**
  * struct genl_family - generic netlink family
  * @id: protocol family idenfitier
  * @hdrsize: length of user specific header in bytes
@@ -14,6 +28,7 @@
  * @attrbuf: buffer to store parsed attributes
  * @ops_list: list of all assigned operations
  * @family_list: family list
+ * @mcast_groups: multicast groups list
  */
 struct genl_family
 {
@@ -25,6 +40,7 @@ struct genl_family
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
+	struct list_head	mcast_groups;	/* private */
 };
 
 /**
@@ -73,6 +89,8 @@ extern int genl_register_family(struct g
 extern int genl_unregister_family(struct genl_family *family);
 extern int genl_register_ops(struct genl_family *, struct genl_ops *ops);
 extern int genl_unregister_ops(struct genl_family *, struct genl_ops *ops);
+extern int genl_register_mc_group(struct genl_family *family,
+				  struct genl_multicast_group *grp);
 
 extern struct sock *genl_sock;
 
--- wireless-dev.orig/net/netlink/genetlink.c	2007-06-26 16:58:42.244806179 +0200
+++ wireless-dev/net/netlink/genetlink.c	2007-06-29 16:48:35.007910932 +0200
@@ -3,6 +3,7 @@
  *
  * 		Authors:	Jamal Hadi Salim
  * 				Thomas Graf <tgraf@suug.ch>
+ *				Johannes Berg <johannes@sipsolutions.net>
  */
 
 #include <linux/module.h>
@@ -13,6 +14,7 @@
 #include <linux/string.h>
 #include <linux/skbuff.h>
 #include <linux/mutex.h>
+#include <linux/bitmap.h>
 #include <net/sock.h>
 #include <net/genetlink.h>
 
@@ -42,6 +44,16 @@ static void genl_unlock(void)
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
 static struct list_head family_ht[GENL_FAM_TAB_SIZE];
+/*
+ * Bitmap of multicast groups that are currently in use.
+ *
+ * To avoid an allocation at boot of just one unsigned long,
+ * declare it global instead.
+ * Bit 1 is marked as already used since group 1 is the controller group.
+ */
+static unsigned long mc_group_start = 0x2;
+static unsigned long *mc_groups = &mc_group_start;
+static unsigned long mc_groups_longs = 1;
 
 static int genl_ctrl_event(int event, void *data);
 
@@ -116,6 +128,59 @@ static inline u16 genl_generate_id(void)
 	return id_gen_idx;
 }
 
+int genl_register_mc_group(struct genl_family *family,
+			   struct genl_multicast_group *grp)
+{
+	int id = find_first_zero_bit(mc_groups,
+				     mc_groups_longs * BITS_PER_LONG);
+	unsigned long *new_groups;
+	size_t nlen = (mc_groups_longs + 1) * sizeof(unsigned long);
+
+	if (id >= mc_groups_longs * BITS_PER_LONG) {
+		if (mc_groups == &mc_group_start) {
+			new_groups = kzalloc(nlen, GFP_KERNEL);
+			if (!mc_groups)
+				return -ENOMEM;
+			mc_groups = new_groups;
+			*mc_groups = mc_group_start;
+		} else {
+			new_groups = krealloc(mc_groups, nlen, GFP_KERNEL);
+			if (!new_groups)
+				return -ENOMEM;
+			mc_groups = new_groups;
+			mc_groups[mc_groups_longs] = 0;
+		}
+		mc_groups_longs++;
+	}
+
+	grp->id = id;
+	set_bit(id, mc_groups);
+	list_add_tail(&grp->list, &family->mcast_groups);
+
+	genl_ctrl_event(CTRL_CMD_NEWMCAST_GRP, family);
+
+	return 0;
+}
+EXPORT_SYMBOL(genl_register_mc_group);
+
+static void genl_unregister_mc_group(struct genl_multicast_group *grp)
+{
+	/*
+	 * TODO: fix multicast group re-use by clearing the bit
+	 *	 for this group in all genetlink sockets.
+	 */
+	clear_bit(grp->id, mc_groups);
+	list_del(&grp->list);
+}
+
+static void genl_unregister_mc_groups(struct genl_family *family)
+{
+	struct genl_multicast_group *grp, *tmp;
+
+	list_for_each_entry_safe(grp, tmp, &family->mcast_groups, list)
+		genl_unregister_mc_group(grp);
+}
+
 /**
  * genl_register_ops - register generic netlink operations
  * @family: generic netlink family
@@ -290,6 +355,8 @@ int genl_unregister_family(struct genl_f
 		return 0;
 	}
 
+	genl_unregister_mc_groups(family);
+
 	genl_unlock();
 
 	return -ENOENT;
@@ -410,6 +477,31 @@ static int ctrl_fill_info(struct genl_fa
 		nla_nest_end(skb, nla_ops);
 	}
 
+	if (!list_empty(&family->mcast_groups)) {
+		struct genl_multicast_group *grp;
+		struct nlattr *nla_grps;
+		int idx = 1;
+
+		nla_grps = nla_nest_start(skb, CTRL_ATTR_MCAST_GROUPS);
+		if (nla_grps == NULL)
+			goto nla_put_failure;
+
+		list_for_each_entry(grp, &family->mcast_groups, list) {
+			struct nlattr *nest;
+
+			nest = nla_nest_start(skb, idx++);
+			if (nest == NULL)
+				goto nla_put_failure;
+
+			NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, grp->id);
+			NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME,
+				       grp->name);
+
+			nla_nest_end(skb, nest);
+		}
+		nla_nest_end(skb, nla_grps);
+	}
+
 	return genlmsg_end(skb, hdr);
 
 nla_put_failure:
@@ -523,6 +615,7 @@ static int genl_ctrl_event(int event, vo
 	switch (event) {
 	case CTRL_CMD_NEWFAMILY:
 	case CTRL_CMD_DELFAMILY:
+	case CTRL_CMD_NEWMCAST_GRP:
 		msg = ctrl_build_msg(data, 0, 0, event);
 		if (IS_ERR(msg))
 			return PTR_ERR(msg);
--- wireless-dev.orig/include/linux/genetlink.h	2007-06-26 16:58:42.174806179 +0200
+++ wireless-dev/include/linux/genetlink.h	2007-06-29 16:00:05.687910932 +0200
@@ -39,6 +39,9 @@ enum {
 	CTRL_CMD_NEWOPS,
 	CTRL_CMD_DELOPS,
 	CTRL_CMD_GETOPS,
+	CTRL_CMD_NEWMCAST_GRP,
+	CTRL_CMD_DELMCAST_GRP, /* unused */
+	CTRL_CMD_GETMCAST_GRP, /* unused */
 	__CTRL_CMD_MAX,
 };
 
@@ -52,6 +55,7 @@ enum {
 	CTRL_ATTR_HDRSIZE,
 	CTRL_ATTR_MAXATTR,
 	CTRL_ATTR_OPS,
+	CTRL_ATTR_MCAST_GROUPS,
 	__CTRL_ATTR_MAX,
 };
 
@@ -66,4 +70,13 @@ enum {
 
 #define CTRL_ATTR_OP_MAX (__CTRL_ATTR_OP_MAX - 1)
 
+enum {
+	CTRL_ATTR_MCAST_GRP_UNSPEC,
+	CTRL_ATTR_MCAST_GRP_NAME,
+	CTRL_ATTR_MCAST_GRP_ID,
+	__CTRL_ATTR_MCAST_GRP_MAX,
+};
+
+#define CTRL_ATTR_MCAST_GRP_MAX (__CTRL_ATTR_MCAST_GRP_MAX - 1)
+
 #endif	/* __LINUX_GENERIC_NETLINK_H */

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[jamal]

On Fri, 2007-29-06 at 16:56 +0200, Johannes Berg wrote:


> +static void genl_unregister_mc_group(struct genl_multicast_group *grp)
> +{
> +	/*
> +	 * TODO: fix multicast group re-use by clearing the bit
> +	 *	 for this group in all genetlink sockets.
> +	 */
> +	clear_bit(grp->id, mc_groups);
> +	list_del(&grp->list);

I think you need a 
genl_ctrl_event(CTRL_CMD_DELMCAST_GRP, family);
here? You may need to save the mcast details before you delete.


> +			NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, grp->id);
> +			NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME,
> +				       grp->name);
> +

Consider my earlier suggestion to use CTRL_ATTR_MCAST_GRP which has both
id and name in one struct.

Other than that - looking good.

cheers,
jamal

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Sat, 2007-06-30 at 11:32 -0400, jamal wrote:

> > +static void genl_unregister_mc_group(struct genl_multicast_group *grp)
> > +{
> > +	/*
> > +	 * TODO: fix multicast group re-use by clearing the bit
> > +	 *	 for this group in all genetlink sockets.
> > +	 */
> > +	clear_bit(grp->id, mc_groups);
> > +	list_del(&grp->list);
> 
> I think you need a 
> genl_ctrl_event(CTRL_CMD_DELMCAST_GRP, family);
> here? You may need to save the mcast details before you delete.

I will, but not currently because right now mcast groups are only
deleted when the family is dropped.

> > +			NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, grp->id);
> > +			NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME,
> > +				       grp->name);
> > +
> 
> Consider my earlier suggestion to use CTRL_ATTR_MCAST_GRP which has both
> id and name in one struct.

Yeah I thought about that but then saw Patrick's patches to convert
other things away from structs so I wasn't sure what the idea here is.
Patrick, care to comment?

> Other than that - looking good.

:)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Sat, 2007-06-30 at 11:32 -0400, jamal wrote:
> 
> 
>>>+			NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, grp->id);
>>>+			NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME,
>>>+				       grp->name);
>>>+
>>
>>Consider my earlier suggestion to use CTRL_ATTR_MCAST_GRP which has both
>>id and name in one struct.
> 
> 
> Yeah I thought about that but then saw Patrick's patches to convert
> other things away from structs so I wasn't sure what the idea here is.
> Patrick, care to comment?


For information that belongs together logically a struct is fine.
The main reason to use nested attributes is when you only have a
single attribute to store your data in (for example TCA_OPTIONS
for qdiscs). In that case a nested attribute should be used to
allow to extend it in the future. Below that nested attribute
you could put a struct of course.

In this case I think using a string attribute instead of a fixed
sized structure also makes sense for a different reason. Its
unlikely that groups will really use the maximum name length
allowed, so it should save some bandwidth.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Mon, 2007-07-02 at 14:56 +0200, Patrick McHardy wrote:

> For information that belongs together logically a struct is fine.

Ok.

> The main reason to use nested attributes is when you only have a
> single attribute to store your data in (for example TCA_OPTIONS
> for qdiscs). In that case a nested attribute should be used to
> allow to extend it in the future. Below that nested attribute
> you could put a struct of course.

Right, but that's not applicable to this unless I'm misunderstanding
you.

> In this case I think using a string attribute instead of a fixed
> sized structure also makes sense for a different reason. Its
> unlikely that groups will really use the maximum name length
> allowed, so it should save some bandwidth.

I suppose if I put (ID,name) into the struct it needn't be fixed-size
length, but I dislike that as well. Do I understand you correctly in
that you prefer the way I did it now?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Mon, 2007-07-02 at 14:56 +0200, Patrick McHardy wrote:
> 
>>The main reason to use nested attributes is when you only have a
>>single attribute to store your data in (for example TCA_OPTIONS
>>for qdiscs). In that case a nested attribute should be used to
>>allow to extend it in the future. Below that nested attribute
>>you could put a struct of course.
> 
> 
> Right, but that's not applicable to this unless I'm misunderstanding
> you.


Not really, it already uses a nested top-level attribute.

>>In this case I think using a string attribute instead of a fixed
>>sized structure also makes sense for a different reason. Its
>>unlikely that groups will really use the maximum name length
>>allowed, so it should save some bandwidth.
> 
> 
> I suppose if I put (ID,name) into the struct it needn't be fixed-size
> length, but I dislike that as well.


Me too.

> Do I understand you correctly in that you prefer the way I did it now?


Yes.

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Mon, 2007-07-02 at 16:38 +0200, Patrick McHardy wrote:

> > Do I understand you correctly in that you prefer the way I did it now?
> 
> 
> Yes.

Alright, then I'll rework the event generation to not include the whole
family info and repost with that tomorrow or so. If I find time I might
actually fix the unregistration bug too, but I have a feeling digging in
the socket code might take more time than I have right now.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Johannes Berg]

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

On Mon, 2007-07-02 at 16:48 +0200, Johannes Berg wrote:

> If I find time I might
> actually fix the unregistration bug too, but I have a feeling digging in
> the socket code might take more time than I have right now.

Hmm. I started digging into the af_netlink.c code and realised that the
whole thing I've been doing cannot possibly work completely since the
genl socket is created with GENL_MAX_ID as the "groups" parameter to
netlink_kernel_create() and that limits the groups, and the af_netlink
code really wants to know the number of groups up-front.

So some deeper surgery is required to lift the limit of 1023 multicast
group now. Not that I like the current genetlink code, we allocate 256
bytes for the in-kernel socket just for the listeners bitmap, and just
as many for each socket's groups bitmaps while it's unlikely a regular
system right now will ever reach that limit.

I'll be posting some patches as replies.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[PATCH] netlink: allocate group bitmaps dynamically

[Johannes Berg]

Allow changing the number of groups for a netlink family
after it has been created.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 include/linux/netlink.h  |    1 
 net/netlink/af_netlink.c |   61 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 47 insertions(+), 15 deletions(-)

--- wireless-dev.orig/net/netlink/af_netlink.c	2007-07-03 00:10:31.617889695 +0200
+++ wireless-dev/net/netlink/af_netlink.c	2007-07-03 00:31:30.267889695 +0200
@@ -316,8 +316,11 @@ netlink_update_listeners(struct sock *sk
 
 	for (i = 0; i < NLGRPSZ(tbl->groups)/sizeof(unsigned long); i++) {
 		mask = 0;
-		sk_for_each_bound(sk, node, &tbl->mc_list)
-			mask |= nlk_sk(sk)->groups[i];
+		sk_for_each_bound(sk, node, &tbl->mc_list) {
+			if (nlk_sk(sk)->ngroups >=
+			    (i + 1) * sizeof(unsigned long))
+				mask |= nlk_sk(sk)->groups[i];
+		}
 		tbl->listeners[i] = mask;
 	}
 	/* this function is only called with the netlink table "grabbed", which
@@ -555,10 +558,11 @@ netlink_update_subscriptions(struct sock
 	nlk->subscriptions = subscriptions;
 }
 
-static int netlink_alloc_groups(struct sock *sk)
+static int netlink_realloc_groups(struct sock *sk)
 {
 	struct netlink_sock *nlk = nlk_sk(sk);
 	unsigned int groups;
+	unsigned long *new_groups;
 	int err = 0;
 
 	netlink_lock_table();
@@ -570,9 +574,15 @@ static int netlink_alloc_groups(struct s
 	if (err)
 		return err;
 
-	nlk->groups = kzalloc(NLGRPSZ(groups), GFP_KERNEL);
-	if (nlk->groups == NULL)
+	if (nlk->ngroups >= groups)
+		return 0;
+
+	new_groups = krealloc(nlk->groups, NLGRPSZ(groups), GFP_KERNEL);
+	if (new_groups == NULL)
 		return -ENOMEM;
+	memset((char*)new_groups + NLGRPSZ(nlk->ngroups), 0,
+	       NLGRPSZ(groups) - NLGRPSZ(nlk->ngroups));
+	nlk->groups = new_groups;
 	nlk->ngroups = groups;
 	return 0;
 }
@@ -591,11 +601,9 @@ static int netlink_bind(struct socket *s
 	if (nladdr->nl_groups) {
 		if (!netlink_capable(sock, NL_NONROOT_RECV))
 			return -EPERM;
-		if (nlk->groups == NULL) {
-			err = netlink_alloc_groups(sk);
-			if (err)
-				return err;
-		}
+		err = netlink_realloc_groups(sk);
+		if (err)
+			return err;
 	}
 
 	if (nlk->pid) {
@@ -1041,11 +1049,9 @@ static int netlink_setsockopt(struct soc
 
 		if (!netlink_capable(sock, NL_NONROOT_RECV))
 			return -EPERM;
-		if (nlk->groups == NULL) {
-			err = netlink_alloc_groups(sk);
-			if (err)
-				return err;
-		}
+		err = netlink_realloc_groups(sk);
+		if (err)
+			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
 		netlink_table_grab();
@@ -1332,6 +1338,31 @@ out_sock_release:
 	return NULL;
 }
 
+int netlink_change_ngroups(int unit, unsigned int groups)
+{
+	unsigned long *listeners;
+	int err = 0;
+
+	netlink_table_grab();
+	if (NLGRPSZ(nl_table[unit].groups) < NLGRPSZ(groups)) {
+		listeners = krealloc(nl_table[unit].listeners,
+				     NLGRPSZ(groups), GFP_KERNEL);
+		if (!listeners) {
+			err = -ENOMEM;
+			goto out_ungrab;
+		}
+		memset((char*) listeners + NLGRPSZ(nl_table[unit].groups),
+		       0, NLGRPSZ(groups) - NLGRPSZ(nl_table[unit].groups));
+		nl_table[unit].listeners = listeners;
+	}
+	nl_table[unit].groups = groups;
+
+ out_ungrab:
+	netlink_table_ungrab();
+	return err;
+}
+EXPORT_SYMBOL(netlink_change_ngroups);
+
 void netlink_set_nonroot(int protocol, unsigned int flags)
 {
 	if ((unsigned int)protocol < MAX_LINKS)
--- wireless-dev.orig/include/linux/netlink.h	2007-07-03 00:09:13.047889695 +0200
+++ wireless-dev/include/linux/netlink.h	2007-07-03 00:31:30.267889695 +0200
@@ -161,6 +161,7 @@ extern struct sock *netlink_kernel_creat
 					  void (*input)(struct sock *sk, int len),
 					  struct mutex *cb_mutex,
 					  struct module *module);
+extern int netlink_change_ngroups(int unit, unsigned int groups);
 extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
 extern int netlink_has_listeners(struct sock *sk, unsigned int group);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);

[PATCH] netlink: allow removing multicast groups

[Johannes Berg]

Allow kicking listeners out of a multicast group when necessary
(for example if that group is going to be removed.)

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 include/linux/netlink.h  |    1 +
 net/netlink/af_netlink.c |   47 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 35 insertions(+), 13 deletions(-)

--- wireless-dev.orig/include/linux/netlink.h	2007-07-03 11:54:21.818696328 +0200
+++ wireless-dev/include/linux/netlink.h	2007-07-03 11:54:32.638696328 +0200
@@ -162,6 +162,7 @@ extern struct sock *netlink_kernel_creat
 					  struct mutex *cb_mutex,
 					  struct module *module);
 extern int netlink_change_ngroups(int unit, unsigned int groups);
+extern void netlink_clear_multicast_users(int unit, unsigned int group);
 extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
 extern int netlink_has_listeners(struct sock *sk, unsigned int group);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);
--- wireless-dev.orig/net/netlink/af_netlink.c	2007-07-03 11:54:21.908696328 +0200
+++ wireless-dev/net/netlink/af_netlink.c	2007-07-03 11:54:32.638696328 +0200
@@ -1015,6 +1015,24 @@ void netlink_set_err(struct sock *ssk, u
 	read_unlock(&nl_table_lock);
 }
 
+static void netlink_update_socket_mc(struct netlink_sock *nlk,
+				     unsigned int group,
+				     int is_new)
+{
+	int old, new = !!is_new, subscriptions;
+
+	netlink_table_grab();
+	old = test_bit(group - 1, nlk->groups);
+	subscriptions = nlk->subscriptions - old + new;
+	if (new)
+		__set_bit(group - 1, nlk->groups);
+	else
+		__clear_bit(group - 1, nlk->groups);
+	netlink_update_subscriptions(&nlk->sk, subscriptions);
+	netlink_update_listeners(&nlk->sk);
+	netlink_table_ungrab();
+}
+
 static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			      char __user *optval, int optlen)
 {
@@ -1040,9 +1058,6 @@ static int netlink_setsockopt(struct soc
 		break;
 	case NETLINK_ADD_MEMBERSHIP:
 	case NETLINK_DROP_MEMBERSHIP: {
-		unsigned int subscriptions;
-		int old, new = optname == NETLINK_ADD_MEMBERSHIP ? 1 : 0;
-
 		if (!netlink_capable(sock, NL_NONROOT_RECV))
 			return -EPERM;
 		err = netlink_realloc_groups(sk);
@@ -1050,16 +1065,8 @@ static int netlink_setsockopt(struct soc
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
-		netlink_table_grab();
-		old = test_bit(val - 1, nlk->groups);
-		subscriptions = nlk->subscriptions - old + new;
-		if (new)
-			__set_bit(val - 1, nlk->groups);
-		else
-			__clear_bit(val - 1, nlk->groups);
-		netlink_update_subscriptions(sk, subscriptions);
-		netlink_update_listeners(sk);
-		netlink_table_ungrab();
+		netlink_update_socket_mc(nlk, val,
+					 optname == NETLINK_ADD_MEMBERSHIP);
 		err = 0;
 		break;
 	}
@@ -1359,6 +1366,20 @@ int netlink_change_ngroups(int unit, uns
 }
 EXPORT_SYMBOL(netlink_change_ngroups);
 
+void netlink_clear_multicast_users(int unit, unsigned int group)
+{
+	struct sock *sk;
+	struct hlist_node *node;
+
+	read_lock(&nl_table_lock);
+
+	sk_for_each_bound(sk, node, &nl_table[unit].mc_list)
+		netlink_update_socket_mc(nlk_sk(sk), group, 0);
+
+	read_unlock(&nl_table_lock);
+}
+EXPORT_SYMBOL(netlink_clear_multicast_users);
+
 void netlink_set_nonroot(int protocol, unsigned int flags)
 {
 	if ((unsigned int)protocol < MAX_LINKS)

[PATCH] generic netlink: dynamic multicast groups

[Johannes Berg]

Introduce API to dynamically register and unregister multicast groups.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Some hackish modifications to iproute2's genl command confirm that it
does indeed work.

 include/linux/genetlink.h |   13 +++
 include/net/genetlink.h   |   22 +++++
 net/netlink/genetlink.c   |  190 ++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 218 insertions(+), 7 deletions(-)

--- wireless-dev.orig/include/net/genetlink.h	2007-07-03 11:49:35.468696328 +0200
+++ wireless-dev/include/net/genetlink.h	2007-07-03 11:49:54.328696328 +0200
@@ -5,6 +5,22 @@
 #include <net/netlink.h>
 
 /**
+ * struct genl_multicast_group - generic netlink multicast group
+ * @name: name of the multicast group, names are per-family
+ * @id: multicast group ID, assigned by the core, to use with
+ *      genlmsg_multicast().
+ * @list: list entry for linking
+ * @family: pointer to family, need not be set before registering
+ */
+struct genl_multicast_group
+{
+	struct genl_family	*family;	/* private */
+	struct list_head	list;		/* private */
+	char			name[GENL_NAMSIZ];
+	u32			id;
+};
+
+/**
  * struct genl_family - generic netlink family
  * @id: protocol family idenfitier
  * @hdrsize: length of user specific header in bytes
@@ -14,6 +30,7 @@
  * @attrbuf: buffer to store parsed attributes
  * @ops_list: list of all assigned operations
  * @family_list: family list
+ * @mcast_groups: multicast groups list
  */
 struct genl_family
 {
@@ -25,6 +42,7 @@ struct genl_family
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
+	struct list_head	mcast_groups;	/* private */
 };
 
 /**
@@ -73,6 +91,10 @@ extern int genl_register_family(struct g
 extern int genl_unregister_family(struct genl_family *family);
 extern int genl_register_ops(struct genl_family *, struct genl_ops *ops);
 extern int genl_unregister_ops(struct genl_family *, struct genl_ops *ops);
+extern int genl_register_mc_group(struct genl_family *family,
+				  struct genl_multicast_group *grp);
+extern void genl_unregister_mc_group(struct genl_family *family,
+				     struct genl_multicast_group *grp);
 
 extern struct sock *genl_sock;
 
--- wireless-dev.orig/net/netlink/genetlink.c	2007-07-03 11:49:35.588696328 +0200
+++ wireless-dev/net/netlink/genetlink.c	2007-07-03 11:49:54.318696328 +0200
@@ -3,6 +3,7 @@
  *
  * 		Authors:	Jamal Hadi Salim
  * 				Thomas Graf <tgraf@suug.ch>
+ *				Johannes Berg <johannes@sipsolutions.net>
  */
 
 #include <linux/module.h>
@@ -13,6 +14,7 @@
 #include <linux/string.h>
 #include <linux/skbuff.h>
 #include <linux/mutex.h>
+#include <linux/bitmap.h>
 #include <net/sock.h>
 #include <net/genetlink.h>
 
@@ -42,6 +44,17 @@ static void genl_unlock(void)
 #define GENL_FAM_TAB_MASK	(GENL_FAM_TAB_SIZE - 1)
 
 static struct list_head family_ht[GENL_FAM_TAB_SIZE];
+/*
+ * Bitmap of multicast groups that are currently in use.
+ *
+ * To avoid an allocation at boot of just one unsigned long,
+ * declare it global instead.
+ * Bit 0 is marked as already used since group 0 is invalid,
+ * bit 1 is marked as already used since group 1 is the controller group.
+ */
+static unsigned long mc_group_start = 0x3;
+static unsigned long *mc_groups = &mc_group_start;
+static unsigned long mc_groups_longs = 1;
 
 static int genl_ctrl_event(int event, void *data);
 
@@ -116,6 +129,77 @@ static inline u16 genl_generate_id(void)
 	return id_gen_idx;
 }
 
+int genl_register_mc_group(struct genl_family *family,
+			   struct genl_multicast_group *grp)
+{
+	int id = find_first_zero_bit(mc_groups,
+				     mc_groups_longs * BITS_PER_LONG);
+	unsigned long *new_groups;
+	size_t nlen = (mc_groups_longs + 1) * sizeof(unsigned long);
+	int err;
+
+	genl_lock();
+
+	if (id >= mc_groups_longs * BITS_PER_LONG) {
+		if (mc_groups == &mc_group_start) {
+			new_groups = kzalloc(nlen, GFP_KERNEL);
+			if (!mc_groups) {
+				err = -ENOMEM;
+				goto out;
+			}
+			mc_groups = new_groups;
+			*mc_groups = mc_group_start;
+		} else {
+			new_groups = krealloc(mc_groups, nlen, GFP_KERNEL);
+			if (!new_groups) {
+				err = -ENOMEM;
+				goto out;
+			}
+			mc_groups = new_groups;
+			mc_groups[mc_groups_longs] = 0;
+		}
+		mc_groups_longs++;
+	}
+
+	err = netlink_change_ngroups(NETLINK_GENERIC,
+				     sizeof(unsigned long) * NETLINK_GENERIC);
+	if (err)
+		goto out;
+
+	grp->id = id;
+	set_bit(id, mc_groups);
+	list_add_tail(&grp->list, &family->mcast_groups);
+	grp->family = family;
+
+	genl_ctrl_event(CTRL_CMD_NEWMCAST_GRP, grp);
+ out:
+	genl_unlock();
+	return 0;
+}
+EXPORT_SYMBOL(genl_register_mc_group);
+
+void genl_unregister_mc_group(struct genl_family *family,
+			      struct genl_multicast_group *grp)
+{
+	BUG_ON(grp->family != family);
+	genl_lock();
+	netlink_clear_multicast_users(NETLINK_GENERIC, grp->id);
+	clear_bit(grp->id, mc_groups);
+	list_del(&grp->list);
+	genl_ctrl_event(CTRL_CMD_DELMCAST_GRP, grp);
+	grp->id = 0;
+	grp->family = NULL;
+	genl_unlock();
+}
+
+static void genl_unregister_mc_groups(struct genl_family *family)
+{
+	struct genl_multicast_group *grp, *tmp;
+
+	list_for_each_entry_safe(grp, tmp, &family->mcast_groups, list)
+		genl_unregister_mc_group(family, grp);
+}
+
 /**
  * genl_register_ops - register generic netlink operations
  * @family: generic netlink family
@@ -216,6 +300,7 @@ int genl_register_family(struct genl_fam
 		goto errout;
 
 	INIT_LIST_HEAD(&family->ops_list);
+	INIT_LIST_HEAD(&family->mcast_groups);
 
 	genl_lock();
 
@@ -275,6 +360,8 @@ int genl_unregister_family(struct genl_f
 {
 	struct genl_family *rc;
 
+	genl_unregister_mc_groups(family);
+
 	genl_lock();
 
 	list_for_each_entry(rc, genl_family_chain(family->id), family_list) {
@@ -410,6 +497,67 @@ static int ctrl_fill_info(struct genl_fa
 		nla_nest_end(skb, nla_ops);
 	}
 
+	if (!list_empty(&family->mcast_groups)) {
+		struct genl_multicast_group *grp;
+		struct nlattr *nla_grps;
+		int idx = 1;
+
+		nla_grps = nla_nest_start(skb, CTRL_ATTR_MCAST_GROUPS);
+		if (nla_grps == NULL)
+			goto nla_put_failure;
+
+		list_for_each_entry(grp, &family->mcast_groups, list) {
+			struct nlattr *nest;
+
+			nest = nla_nest_start(skb, idx++);
+			if (nest == NULL)
+				goto nla_put_failure;
+
+			NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, grp->id);
+			NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME,
+				       grp->name);
+
+			nla_nest_end(skb, nest);
+		}
+		nla_nest_end(skb, nla_grps);
+	}
+
+	return genlmsg_end(skb, hdr);
+
+nla_put_failure:
+	return genlmsg_cancel(skb, hdr);
+}
+
+static int ctrl_fill_mcgrp_info(struct genl_multicast_group *grp, u32 pid,
+				u32 seq, u32 flags, struct sk_buff *skb,
+				u8 cmd)
+{
+	void *hdr;
+	struct nlattr *nla_grps;
+	struct nlattr *nest;
+
+	hdr = genlmsg_put(skb, pid, seq, &genl_ctrl, flags, cmd);
+	if (hdr == NULL)
+		return -1;
+
+	NLA_PUT_STRING(skb, CTRL_ATTR_FAMILY_NAME, grp->family->name);
+	NLA_PUT_U16(skb, CTRL_ATTR_FAMILY_ID, grp->family->id);
+
+	nla_grps = nla_nest_start(skb, CTRL_ATTR_MCAST_GROUPS);
+	if (nla_grps == NULL)
+		goto nla_put_failure;
+
+	nest = nla_nest_start(skb, 1);
+	if (nest == NULL)
+		goto nla_put_failure;
+
+	NLA_PUT_U32(skb, CTRL_ATTR_MCAST_GRP_ID, grp->id);
+	NLA_PUT_STRING(skb, CTRL_ATTR_MCAST_GRP_NAME,
+		       grp->name);
+
+	nla_nest_end(skb, nest);
+	nla_nest_end(skb, nla_grps);
+
 	return genlmsg_end(skb, hdr);
 
 nla_put_failure:
@@ -453,8 +601,8 @@ errout:
 	return skb->len;
 }
 
-static struct sk_buff *ctrl_build_msg(struct genl_family *family, u32 pid,
-				      int seq, u8 cmd)
+static struct sk_buff *ctrl_build_family_msg(struct genl_family *family,
+					     u32 pid, int seq, u8 cmd)
 {
 	struct sk_buff *skb;
 	int err;
@@ -472,6 +620,25 @@ static struct sk_buff *ctrl_build_msg(st
 	return skb;
 }
 
+static struct sk_buff *ctrl_build_mcgrp_msg(struct genl_multicast_group *grp,
+					    u32 pid, int seq, u8 cmd)
+{
+	struct sk_buff *skb;
+	int err;
+
+	skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (skb == NULL)
+		return ERR_PTR(-ENOBUFS);
+
+	err = ctrl_fill_mcgrp_info(grp, pid, seq, 0, skb, cmd);
+	if (err < 0) {
+		nlmsg_free(skb);
+		return ERR_PTR(err);
+	}
+
+	return skb;
+}
+
 static const struct nla_policy ctrl_policy[CTRL_ATTR_MAX+1] = {
 	[CTRL_ATTR_FAMILY_ID]	= { .type = NLA_U16 },
 	[CTRL_ATTR_FAMILY_NAME]	= { .type = NLA_NUL_STRING,
@@ -501,8 +668,8 @@ static int ctrl_getfamily(struct sk_buff
 		goto errout;
 	}
 
-	msg = ctrl_build_msg(res, info->snd_pid, info->snd_seq,
-			     CTRL_CMD_NEWFAMILY);
+	msg = ctrl_build_family_msg(res, info->snd_pid, info->snd_seq,
+				    CTRL_CMD_NEWFAMILY);
 	if (IS_ERR(msg)) {
 		err = PTR_ERR(msg);
 		goto errout;
@@ -523,7 +690,15 @@ static int genl_ctrl_event(int event, vo
 	switch (event) {
 	case CTRL_CMD_NEWFAMILY:
 	case CTRL_CMD_DELFAMILY:
-		msg = ctrl_build_msg(data, 0, 0, event);
+		msg = ctrl_build_family_msg(data, 0, 0, event);
+		if (IS_ERR(msg))
+			return PTR_ERR(msg);
+
+		genlmsg_multicast(msg, 0, GENL_ID_CTRL, GFP_KERNEL);
+		break;
+	case CTRL_CMD_NEWMCAST_GRP:
+	case CTRL_CMD_DELMCAST_GRP:
+		msg = ctrl_build_mcgrp_msg(data, 0, 0, event);
 		if (IS_ERR(msg))
 			return PTR_ERR(msg);
 
@@ -557,8 +732,9 @@ static int __init genl_init(void)
 		goto errout_register;
 
 	netlink_set_nonroot(NETLINK_GENERIC, NL_NONROOT_RECV);
-	genl_sock = netlink_kernel_create(NETLINK_GENERIC, GENL_MAX_ID,
-					  genl_rcv, NULL, THIS_MODULE);
+	/* only one static multicast group */
+	genl_sock = netlink_kernel_create(NETLINK_GENERIC, 1, genl_rcv,
+					  NULL, THIS_MODULE);
 	if (genl_sock == NULL)
 		panic("GENL: Cannot initialize generic netlink\n");
 
--- wireless-dev.orig/include/linux/genetlink.h	2007-07-03 11:49:35.498696328 +0200
+++ wireless-dev/include/linux/genetlink.h	2007-07-03 11:49:54.328696328 +0200
@@ -39,6 +39,9 @@ enum {
 	CTRL_CMD_NEWOPS,
 	CTRL_CMD_DELOPS,
 	CTRL_CMD_GETOPS,
+	CTRL_CMD_NEWMCAST_GRP,
+	CTRL_CMD_DELMCAST_GRP,
+	CTRL_CMD_GETMCAST_GRP, /* unused */
 	__CTRL_CMD_MAX,
 };
 
@@ -52,6 +55,7 @@ enum {
 	CTRL_ATTR_HDRSIZE,
 	CTRL_ATTR_MAXATTR,
 	CTRL_ATTR_OPS,
+	CTRL_ATTR_MCAST_GROUPS,
 	__CTRL_ATTR_MAX,
 };
 
@@ -66,4 +70,13 @@ enum {
 
 #define CTRL_ATTR_OP_MAX (__CTRL_ATTR_OP_MAX - 1)
 
+enum {
+	CTRL_ATTR_MCAST_GRP_UNSPEC,
+	CTRL_ATTR_MCAST_GRP_NAME,
+	CTRL_ATTR_MCAST_GRP_ID,
+	__CTRL_ATTR_MCAST_GRP_MAX,
+};
+
+#define CTRL_ATTR_MCAST_GRP_MAX (__CTRL_ATTR_MCAST_GRP_MAX - 1)
+
 #endif	/* __LINUX_GENERIC_NETLINK_H */

Re: Fwd: [PATCH] [-mm] ACPI: export ACPI events via netlink

[Patrick McHardy]

Johannes Berg wrote:
> On Mon, 2007-07-02 at 16:48 +0200, Johannes Berg wrote:
> 
> 
>>If I find time I might
>>actually fix the unregistration bug too, but I have a feeling digging in
>>the socket code might take more time than I have right now.
> 
> 
> Hmm. I started digging into the af_netlink.c code and realised that the
> whole thing I've been doing cannot possibly work completely since the
> genl socket is created with GENL_MAX_ID as the "groups" parameter to
> netlink_kernel_create() and that limits the groups, and the af_netlink
> code really wants to know the number of groups up-front.


Good point, I missed that.

> So some deeper surgery is required to lift the limit of 1023 multicast
> group now. Not that I like the current genetlink code, we allocate 256
> bytes for the in-kernel socket just for the listeners bitmap, and just
> as many for each socket's groups bitmaps while it's unlikely a regular
> system right now will ever reach that limit.


The kernel socket doesn't have a bitmap allocated, only user sockets
have, and only if they actually bind to a multicast group. In case
of genetlink that still almost as bad of course.

Re: [PATCH] netlink: allocate group bitmaps dynamically

[Patrick McHardy]

Johannes Berg wrote:
> Allow changing the number of groups for a netlink family
> after it has been created.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> 
> ---
>  include/linux/netlink.h  |    1 
>  net/netlink/af_netlink.c |   61 +++++++++++++++++++++++++++++++++++------------
>  2 files changed, 47 insertions(+), 15 deletions(-)
> 
> --- wireless-dev.orig/net/netlink/af_netlink.c	2007-07-03 00:10:31.617889695 +0200
> +++ wireless-dev/net/netlink/af_netlink.c	2007-07-03 00:31:30.267889695 +0200
> @@ -316,8 +316,11 @@ netlink_update_listeners(struct sock *sk
>  
>  	for (i = 0; i < NLGRPSZ(tbl->groups)/sizeof(unsigned long); i++) {
>  		mask = 0;
> -		sk_for_each_bound(sk, node, &tbl->mc_list)
> -			mask |= nlk_sk(sk)->groups[i];
> +		sk_for_each_bound(sk, node, &tbl->mc_list) {
> +			if (nlk_sk(sk)->ngroups >=
> +			    (i + 1) * sizeof(unsigned long))


This condition implies that a socket can bind to a non-existant
group, which shouldn't be possible.

> +				mask |= nlk_sk(sk)->groups[i];
> +		}
>  		tbl->listeners[i] = mask;
>  	}
>  	/* this function is only called with the netlink table "grabbed", which
> @@ -555,10 +558,11 @@ netlink_update_subscriptions(struct sock
>  	nlk->subscriptions = subscriptions;
>  }
>  
> -static int netlink_alloc_groups(struct sock *sk)
> +static int netlink_realloc_groups(struct sock *sk)
>  {
>  	struct netlink_sock *nlk = nlk_sk(sk);
>  	unsigned int groups;
> +	unsigned long *new_groups;
>  	int err = 0;
>  
>  	netlink_lock_table();
> @@ -570,9 +574,15 @@ static int netlink_alloc_groups(struct s
>  	if (err)
>  		return err;
>  
> -	nlk->groups = kzalloc(NLGRPSZ(groups), GFP_KERNEL);
> -	if (nlk->groups == NULL)
> +	if (nlk->ngroups >= groups)
> +		return 0;
> +
> +	new_groups = krealloc(nlk->groups, NLGRPSZ(groups), GFP_KERNEL);
> +	if (new_groups == NULL)
>  		return -ENOMEM;
> +	memset((char*)new_groups + NLGRPSZ(nlk->ngroups), 0,
> +	       NLGRPSZ(groups) - NLGRPSZ(nlk->ngroups));
> +	nlk->groups = new_groups;


This should probably happen with the table grabbed to avoid races
with concurrent broadcasts.



> @@ -1332,6 +1338,31 @@ out_sock_release:
>  	return NULL;
>  }
>  
> +int netlink_change_ngroups(int unit, unsigned int groups)
> +{
> +	unsigned long *listeners;
> +	int err = 0;
> +
> +	netlink_table_grab();


Unfortunately that doesn't prevent races with netlink_has_listeners
since its lockless (and should really stay that way).


> +	if (NLGRPSZ(nl_table[unit].groups) < NLGRPSZ(groups)) {
> +		listeners = krealloc(nl_table[unit].listeners,
> +				     NLGRPSZ(groups), GFP_KERNEL);
> +		if (!listeners) {
> +			err = -ENOMEM;
> +			goto out_ungrab;
> +		}
> +		memset((char*) listeners + NLGRPSZ(nl_table[unit].groups),
> +		       0, NLGRPSZ(groups) - NLGRPSZ(nl_table[unit].groups));
> +		nl_table[unit].listeners = listeners;
> +	}
> +	nl_table[unit].groups = groups;
> +
> + out_ungrab:
> +	netlink_table_ungrab();
> +	return err;
> +}
> +EXPORT_SYMBOL(netlink_change_ngroups);
> +
>  void netlink_set_nonroot(int protocol, unsigned int flags)
>  {
>  	if ((unsigned int)protocol < MAX_LINKS)
> --- wireless-dev.orig/include/linux/netlink.h	2007-07-03 00:09:13.047889695 +0200
> +++ wireless-dev/include/linux/netlink.h	2007-07-03 00:31:30.267889695 +0200
> @@ -161,6 +161,7 @@ extern struct sock *netlink_kernel_creat
>  					  void (*input)(struct sock *sk, int len),
>  					  struct mutex *cb_mutex,
>  					  struct module *module);
> +extern int netlink_change_ngroups(int unit, unsigned int groups);
>  extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
>  extern int netlink_has_listeners(struct sock *sk, unsigned int group);
>  extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);
> 
> 

Re: [PATCH] netlink: allocate group bitmaps dynamically

[Johannes Berg]

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

On Tue, 2007-07-03 at 14:05 +0200, Patrick McHardy wrote:

> > --- wireless-dev.orig/net/netlink/af_netlink.c	2007-07-03 00:10:31.617889695 +0200
> > +++ wireless-dev/net/netlink/af_netlink.c	2007-07-03 00:31:30.267889695 +0200
> > @@ -316,8 +316,11 @@ netlink_update_listeners(struct sock *sk
> >  
> >  	for (i = 0; i < NLGRPSZ(tbl->groups)/sizeof(unsigned long); i++) {
> >  		mask = 0;
> > -		sk_for_each_bound(sk, node, &tbl->mc_list)
> > -			mask |= nlk_sk(sk)->groups[i];
> > +		sk_for_each_bound(sk, node, &tbl->mc_list) {
> > +			if (nlk_sk(sk)->ngroups >=
> > +			    (i + 1) * sizeof(unsigned long))
> 
> 
> This condition implies that a socket can bind to a non-existant
> group, which shouldn't be possible.

Actually, it's the other way around, the socket can bind to group 10
only 32 groups are present (one unsigned long) and then some other code
goes to add groups increasing the limit to 64, and then the socket still
only has a bitmap with 32 bits (one unsigned long) and we shouldn't
access beyond that just because the number of groups was increased.

However, you can in fact bind non-existing groups as long as the group
number is lower than the maximum, i.e. if you start out with just one
group as genetlink does, the netlink code increases that to 32 and you
can bind group 25 even if generic netlink doesn't know about it yet. I
plan to fix that when it actually matters, i.e. when I introduce
per-group permission checks.

> > +				mask |= nlk_sk(sk)->groups[i];
> > +		}
> >  		tbl->listeners[i] = mask;
> >  	}
> >  	/* this function is only called with the netlink table "grabbed", which
> > @@ -555,10 +558,11 @@ netlink_update_subscriptions(struct sock
> >  	nlk->subscriptions = subscriptions;
> >  }
> >  
> > -static int netlink_alloc_groups(struct sock *sk)
> > +static int netlink_realloc_groups(struct sock *sk)
> >  {
> >  	struct netlink_sock *nlk = nlk_sk(sk);
> >  	unsigned int groups;
> > +	unsigned long *new_groups;
> >  	int err = 0;
> >  
> >  	netlink_lock_table();
> > @@ -570,9 +574,15 @@ static int netlink_alloc_groups(struct s
> >  	if (err)
> >  		return err;
> >  
> > -	nlk->groups = kzalloc(NLGRPSZ(groups), GFP_KERNEL);
> > -	if (nlk->groups == NULL)
> > +	if (nlk->ngroups >= groups)
> > +		return 0;
> > +
> > +	new_groups = krealloc(nlk->groups, NLGRPSZ(groups), GFP_KERNEL);
> > +	if (new_groups == NULL)
> >  		return -ENOMEM;
> > +	memset((char*)new_groups + NLGRPSZ(nlk->ngroups), 0,
> > +	       NLGRPSZ(groups) - NLGRPSZ(nlk->ngroups));
> > +	nlk->groups = new_groups;
> 
> 
> This should probably happen with the table grabbed to avoid races
> with concurrent broadcasts.

Hmm, possibly, I'll have to look again.

> > +int netlink_change_ngroups(int unit, unsigned int groups)
> > +{
> > +	unsigned long *listeners;
> > +	int err = 0;
> > +
> > +	netlink_table_grab();
> 
> 
> Unfortunately that doesn't prevent races with netlink_has_listeners
> since its lockless (and should really stay that way).

Uh huh. Good point, I guess I'll have to use RCU or such here when
changing the listeners bitmap size.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] netlink: allocate group bitmaps dynamically

[Patrick McHardy]

Johannes Berg wrote:
> On Tue, 2007-07-03 at 14:05 +0200, Patrick McHardy wrote:
> 
> 
>>>--- wireless-dev.orig/net/netlink/af_netlink.c	2007-07-03 00:10:31.617889695 +0200
>>>+++ wireless-dev/net/netlink/af_netlink.c	2007-07-03 00:31:30.267889695 +0200
>>>@@ -316,8 +316,11 @@ netlink_update_listeners(struct sock *sk
>>> 
>>> 	for (i = 0; i < NLGRPSZ(tbl->groups)/sizeof(unsigned long); i++) {
>>> 		mask = 0;
>>>-		sk_for_each_bound(sk, node, &tbl->mc_list)
>>>-			mask |= nlk_sk(sk)->groups[i];
>>>+		sk_for_each_bound(sk, node, &tbl->mc_list) {
>>>+			if (nlk_sk(sk)->ngroups >=
>>>+			    (i + 1) * sizeof(unsigned long))
>>
>>
>>This condition implies that a socket can bind to a non-existant
>>group, which shouldn't be possible.
> 
> 
> Actually, it's the other way around, the socket can bind to group 10
> only 32 groups are present (one unsigned long) and then some other code
> goes to add groups increasing the limit to 64, and then the socket still
> only has a bitmap with 32 bits (one unsigned long) and we shouldn't
> access beyond that just because the number of groups was increased.


You're right, I misread this code.

> However, you can in fact bind non-existing groups as long as the group
> number is lower than the maximum, i.e. if you start out with just one
> group as genetlink does, the netlink code increases that to 32 and you
> can bind group 25 even if generic netlink doesn't know about it yet. I
> plan to fix that when it actually matters, i.e. when I introduce
> per-group permission checks.


Yes, I was thinking of groups > 32.

> 
> 
>>>+				mask |= nlk_sk(sk)->groups[i];
>>>+		}
>>> 		tbl->listeners[i] = mask;
>>> 	}
>>> 	/* this function is only called with the netlink table "grabbed", which
>>>@@ -555,10 +558,11 @@ netlink_update_subscriptions(struct sock
>>> 	nlk->subscriptions = subscriptions;
>>> }
>>> 
>>>-static int netlink_alloc_groups(struct sock *sk)
>>>+static int netlink_realloc_groups(struct sock *sk)
>>> {
>>> 	struct netlink_sock *nlk = nlk_sk(sk);
>>> 	unsigned int groups;
>>>+	unsigned long *new_groups;
>>> 	int err = 0;
>>> 
>>> 	netlink_lock_table();
>>>@@ -570,9 +574,15 @@ static int netlink_alloc_groups(struct s
>>> 	if (err)
>>> 		return err;
>>> 
>>>-	nlk->groups = kzalloc(NLGRPSZ(groups), GFP_KERNEL);
>>>-	if (nlk->groups == NULL)
>>>+	if (nlk->ngroups >= groups)
>>>+		return 0;
>>>+
>>>+	new_groups = krealloc(nlk->groups, NLGRPSZ(groups), GFP_KERNEL);
>>>+	if (new_groups == NULL)
>>> 		return -ENOMEM;
>>>+	memset((char*)new_groups + NLGRPSZ(nlk->ngroups), 0,
>>>+	       NLGRPSZ(groups) - NLGRPSZ(nlk->ngroups));
>>>+	nlk->groups = new_groups;
>>
>>
>>This should probably happen with the table grabbed to avoid races
>>with concurrent broadcasts.
> 
> 
> Hmm, possibly, I'll have to look again.


do_one_broadcast locks the table and checks nlk->groups. The
reallocation races with this without taking the lock or maybe
using rcu.

> 
> 
>>>+int netlink_change_ngroups(int unit, unsigned int groups)
>>>+{
>>>+	unsigned long *listeners;
>>>+	int err = 0;
>>>+
>>>+	netlink_table_grab();
>>
>>
>>Unfortunately that doesn't prevent races with netlink_has_listeners
>>since its lockless (and should really stay that way).
> 
> 
> Uh huh. Good point, I guess I'll have to use RCU or such here when
> changing the listeners bitmap size.


That should work.

Re: [PATCH] netlink: allocate group bitmaps dynamically

[Johannes Berg]

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

On Tue, 2007-07-03 at 16:11 +0200, Patrick McHardy wrote:


> >>>-	nlk->groups = kzalloc(NLGRPSZ(groups), GFP_KERNEL);
> >>>-	if (nlk->groups == NULL)
> >>>+	if (nlk->ngroups >= groups)
> >>>+		return 0;
> >>>+
> >>>+	new_groups = krealloc(nlk->groups, NLGRPSZ(groups), GFP_KERNEL);
> >>>+	if (new_groups == NULL)
> >>> 		return -ENOMEM;
> >>>+	memset((char*)new_groups + NLGRPSZ(nlk->ngroups), 0,
> >>>+	       NLGRPSZ(groups) - NLGRPSZ(nlk->ngroups));
> >>>+	nlk->groups = new_groups;
> >>
> >>
> >>This should probably happen with the table grabbed to avoid races
> >>with concurrent broadcasts.
> > 
> > 
> > Hmm, possibly, I'll have to look again.
> 
> 
> do_one_broadcast locks the table and checks nlk->groups. The
> reallocation races with this without taking the lock or maybe
> using rcu.

Yeah, sounds about right, but since we lock the table anyway I see
little use in using RCU here. I'll look into it and also double-check
the first hunk of this patch.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[PATCH] netlink: allocate group bitmaps dynamically

[Johannes Berg]

Allow changing the number of groups for a netlink family
after it has been created, use RCU to protect the listeners
bitmap keeping netlink_has_listeners() lock-free.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 include/linux/netlink.h  |    1 
 net/netlink/af_netlink.c |   86 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 66 insertions(+), 21 deletions(-)

--- wireless-dev.orig/net/netlink/af_netlink.c	2007-07-03 12:21:52.161407395 +0200
+++ wireless-dev/net/netlink/af_netlink.c	2007-07-03 18:31:12.371730691 +0200
@@ -62,6 +62,7 @@
 #include <net/netlink.h>
 
 #define NLGRPSZ(x)	(ALIGN(x, sizeof(unsigned long) * 8) / 8)
+#define NLGRPLONGS(x)	(NLGRPSZ(x)/sizeof(unsigned long))
 
 struct netlink_sock {
 	/* struct sock has to be the first member of netlink_sock */
@@ -314,10 +315,12 @@ netlink_update_listeners(struct sock *sk
 	unsigned long mask;
 	unsigned int i;
 
-	for (i = 0; i < NLGRPSZ(tbl->groups)/sizeof(unsigned long); i++) {
+	for (i = 0; i < NLGRPLONGS(tbl->groups); i++) {
 		mask = 0;
-		sk_for_each_bound(sk, node, &tbl->mc_list)
-			mask |= nlk_sk(sk)->groups[i];
+		sk_for_each_bound(sk, node, &tbl->mc_list) {
+			if (i < NLGRPLONGS(nlk_sk(sk)->ngroups))
+				mask |= nlk_sk(sk)->groups[i];
+		}
 		tbl->listeners[i] = mask;
 	}
 	/* this function is only called with the netlink table "grabbed", which
@@ -555,26 +558,37 @@ netlink_update_subscriptions(struct sock
 	nlk->subscriptions = subscriptions;
 }
 
-static int netlink_alloc_groups(struct sock *sk)
+static int netlink_realloc_groups(struct sock *sk)
 {
 	struct netlink_sock *nlk = nlk_sk(sk);
 	unsigned int groups;
+	unsigned long *new_groups;
 	int err = 0;
 
 	netlink_lock_table();
 	groups = nl_table[sk->sk_protocol].groups;
 	if (!nl_table[sk->sk_protocol].registered)
 		err = -ENOENT;
-	netlink_unlock_table();
 
 	if (err)
-		return err;
+		goto out_unlock;
 
-	nlk->groups = kzalloc(NLGRPSZ(groups), GFP_KERNEL);
-	if (nlk->groups == NULL)
-		return -ENOMEM;
+	if (nlk->ngroups >= groups)
+		goto out_unlock;
+
+	new_groups = krealloc(nlk->groups, NLGRPSZ(groups), GFP_KERNEL);
+	if (new_groups == NULL) {
+		err = -ENOMEM;
+		goto out_unlock;
+	}
+	memset((char*)new_groups + NLGRPSZ(nlk->ngroups), 0,
+	       NLGRPSZ(groups) - NLGRPSZ(nlk->ngroups));
+
+	nlk->groups = new_groups;
 	nlk->ngroups = groups;
-	return 0;
+ out_unlock:
+	netlink_unlock_table();
+	return err;
 }
 
 static int netlink_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
@@ -591,11 +605,9 @@ static int netlink_bind(struct socket *s
 	if (nladdr->nl_groups) {
 		if (!netlink_capable(sock, NL_NONROOT_RECV))
 			return -EPERM;
-		if (nlk->groups == NULL) {
-			err = netlink_alloc_groups(sk);
-			if (err)
-				return err;
-		}
+		err = netlink_realloc_groups(sk);
+		if (err)
+			return err;
 	}
 
 	if (nlk->pid) {
@@ -839,10 +851,18 @@ retry:
 int netlink_has_listeners(struct sock *sk, unsigned int group)
 {
 	int res = 0;
+	unsigned long *listeners;
 
 	BUG_ON(!(nlk_sk(sk)->flags & NETLINK_KERNEL_SOCKET));
+
+	rcu_read_lock();
+	listeners = rcu_dereference(nl_table[sk->sk_protocol].listeners);
+
 	if (group - 1 < nl_table[sk->sk_protocol].groups)
-		res = test_bit(group - 1, nl_table[sk->sk_protocol].listeners);
+		res = test_bit(group - 1, listeners);
+
+	rcu_read_unlock();
+
 	return res;
 }
 EXPORT_SYMBOL_GPL(netlink_has_listeners);
@@ -1037,11 +1057,9 @@ static int netlink_setsockopt(struct soc
 
 		if (!netlink_capable(sock, NL_NONROOT_RECV))
 			return -EPERM;
-		if (nlk->groups == NULL) {
-			err = netlink_alloc_groups(sk);
-			if (err)
-				return err;
-		}
+		err = netlink_realloc_groups(sk);
+		if (err)
+			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
 		netlink_table_grab();
@@ -1328,6 +1346,32 @@ out_sock_release:
 	return NULL;
 }
 
+int netlink_change_ngroups(int unit, unsigned int groups)
+{
+	unsigned long *listeners, old = NULL;
+	int err = 0;
+
+	netlink_table_grab();
+	if (NLGRPSZ(nl_table[unit].groups) < NLGRPSZ(groups)) {
+		listeners = kzalloc(NLGRPSZ(groups), GFP_ATOMIC);
+		if (!listeners) {
+			err = -ENOMEM;
+			goto out_ungrab;
+		}
+		old = nl_table[unit].listeners;
+		memcpy(listeners, old, NLGRPSZ(nl_table[unit].groups));
+		rcu_assign_pointer(nl_table[unit].listeners, listeners);
+	}
+	nl_table[unit].groups = groups;
+
+ out_ungrab:
+	netlink_table_ungrab();
+	synchronize_rcu();
+	kfree(old);
+	return err;
+}
+EXPORT_SYMBOL(netlink_change_ngroups);
+
 void netlink_set_nonroot(int protocol, unsigned int flags)
 {
 	if ((unsigned int)protocol < MAX_LINKS)
--- wireless-dev.orig/include/linux/netlink.h	2007-07-03 12:21:50.401407395 +0200
+++ wireless-dev/include/linux/netlink.h	2007-07-03 18:30:49.831730691 +0200
@@ -161,6 +161,7 @@ extern struct sock *netlink_kernel_creat
 					  void (*input)(struct sock *sk, int len),
 					  struct mutex *cb_mutex,
 					  struct module *module);
+extern int netlink_change_ngroups(int unit, unsigned int groups);
 extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
 extern int netlink_has_listeners(struct sock *sk, unsigned int group);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);