Hi Dave, hi Ingo,
patch below switches 802.2 and SNAP layer to use rwlock instead
of cli/sti. If you think that read_lock() has too much overhead
in this code path, I'm sorry, I have no better solution...
It also fixes race between two tasks doing register_*_client
at once - there was no locking here.
If you agree, tell me and I'll forward it to Linus. Or do so
yourself.
Thanks,
Petr Vandrovec
[email protected]
diff -urdN linux/net/802/p8022.c linux/net/802/p8022.c
--- linux/net/802/p8022.c 2002-07-22 13:32:52.000000000 +0000
+++ linux/net/802/p8022.c 2002-07-22 19:32:20.000000000 +0000
@@ -31,6 +31,8 @@
extern void llc_unregister_sap(unsigned char sap);
static struct datalink_proto *p8022_list;
+static rwlock_t p8022_list_lock = RW_LOCK_UNLOCKED;
+
/*
* We don't handle the loopback SAP stuff, the extended
* 802.2 command set, multicast SAP identifiers and non UI
@@ -53,6 +55,7 @@
struct datalink_proto *proto;
int rc = 0;
+ read_lock(&p8022_list_lock);
proto = find_8022_client(*(skb->h.raw));
if (!proto) {
skb->sk = NULL;
@@ -63,7 +66,8 @@
skb->nh.raw += 3;
skb_pull(skb, 3);
rc = proto->rcvfunc(skb, dev, pt);
-out: return rc;
+out: read_unlock(&p8022_list_lock);
+ return rc;
}
static void p8022_datalink_header(struct datalink_proto *dl,
@@ -84,7 +88,9 @@
struct packet_type *))
{
struct datalink_proto *proto = NULL;
+ unsigned long flags;
+ write_lock_irqsave(&p8022_list_lock, flags);
if (find_8022_client(type))
goto out;
proto = kmalloc(sizeof(*proto), GFP_ATOMIC);
@@ -99,7 +105,8 @@
p8022_list = proto;
llc_register_sap(type, p8022_rcv);
}
-out: return proto;
+out: write_unlock_irqrestore(&p8022_list_lock, flags);
+ return proto;
}
void unregister_8022_client(unsigned char type)
@@ -107,8 +114,7 @@
struct datalink_proto *tmp, **clients = &p8022_list;
unsigned long flags;
- save_flags(flags);
- cli();
+ write_lock_irqsave(&p8022_list_lock, flags);
while (*clients) {
tmp = *clients;
if (tmp->type[0] == type) {
@@ -119,7 +125,7 @@
}
clients = &tmp->next;
}
- restore_flags(flags);
+ write_unlock_irqrestore(&p8022_list_lock, flags);
}
EXPORT_SYMBOL(register_8022_client);
diff -urdN linux/net/802/psnap.c linux/net/802/psnap.c
--- linux/net/802/psnap.c 2002-07-22 13:32:58.000000000 +0000
+++ linux/net/802/psnap.c 2002-07-22 19:36:43.000000000 +0000
@@ -22,6 +22,7 @@
static struct datalink_proto *snap_list = NULL;
static struct datalink_proto *snap_dl = NULL; /* 802.2 DL for SNAP */
+static rwlock_t snap_list_lock = RW_LOCK_UNLOCKED;
/*
* Find a snap client by matching the 5 bytes.
@@ -52,9 +53,12 @@
struct datalink_proto *proto;
+ read_lock(&snap_list_lock);
proto = find_snap_client(skb->h.raw);
if (proto != NULL)
{
+ int err;
+
/*
* Pass the frame on.
*/
@@ -65,7 +69,9 @@
if (psnap_packet_type.type == 0)
psnap_packet_type.type=htons(ETH_P_SNAP);
- return proto->rcvfunc(skb, dev, &psnap_packet_type);
+ err = proto->rcvfunc(skb, dev, &psnap_packet_type);
+ read_unlock(&snap_list_lock);
+ return err;
}
skb->sk = NULL;
kfree_skb(skb);
@@ -104,10 +110,12 @@
struct datalink_proto *register_snap_client(unsigned char *desc, int (*rcvfunc)(struct sk_buff *, struct net_device *, struct packet_type *))
{
- struct datalink_proto *proto;
+ struct datalink_proto *proto = NULL;
+ unsigned long flags;
+ write_lock_irqsave(&snap_list_lock, flags);
if (find_snap_client(desc) != NULL)
- return NULL;
+ goto out;
proto = (struct datalink_proto *) kmalloc(sizeof(*proto), GFP_ATOMIC);
if (proto != NULL)
@@ -121,7 +129,7 @@
proto->next = snap_list;
snap_list = proto;
}
-
+out: write_unlock_irqrestore(&snap_list_lock, flags);
return proto;
}
@@ -135,8 +143,7 @@
struct datalink_proto *tmp;
unsigned long flags;
- save_flags(flags);
- cli();
+ write_lock_irqsave(&snap_list_lock, flags);
while ((tmp = *clients) != NULL)
{
@@ -150,5 +157,5 @@
clients = &tmp->next;
}
- restore_flags(flags);
+ write_unlock_irqrestore(&snap_list_lock, flags);
}
On Mon, Jul 22, 2002 at 09:48:14PM +0200, Petr Vandrovec wrote:
> Hi Dave, hi Ingo,
> patch below switches 802.2 and SNAP layer to use rwlock instead
> of cli/sti. If you think that read_lock() has too much overhead
> in this code path, I'm sorry, I have no better solution...
>
> It also fixes race between two tasks doing register_*_client
> at once - there was no locking here.
>
> If you agree, tell me and I'll forward it to Linus. Or do so
> yourself.
> Thanks,
> Petr Vandrovec
> [email protected]
Oops. Older version was missing read_unlock in one of psnap.c error
paths. Fixed version follows.
Petr Vandrovec
diff -urdN linux/net/802/p8022.c linux/net/802/p8022.c
--- linux/net/802/p8022.c 2002-07-22 13:32:52.000000000 +0000
+++ linux/net/802/p8022.c 2002-07-22 19:32:20.000000000 +0000
@@ -31,6 +31,8 @@
extern void llc_unregister_sap(unsigned char sap);
static struct datalink_proto *p8022_list;
+static rwlock_t p8022_list_lock = RW_LOCK_UNLOCKED;
+
/*
* We don't handle the loopback SAP stuff, the extended
* 802.2 command set, multicast SAP identifiers and non UI
@@ -53,6 +55,7 @@
struct datalink_proto *proto;
int rc = 0;
+ read_lock(&p8022_list_lock);
proto = find_8022_client(*(skb->h.raw));
if (!proto) {
skb->sk = NULL;
@@ -63,7 +66,8 @@
skb->nh.raw += 3;
skb_pull(skb, 3);
rc = proto->rcvfunc(skb, dev, pt);
-out: return rc;
+out: read_unlock(&p8022_list_lock);
+ return rc;
}
static void p8022_datalink_header(struct datalink_proto *dl,
@@ -84,7 +88,9 @@
struct packet_type *))
{
struct datalink_proto *proto = NULL;
+ unsigned long flags;
+ write_lock_irqsave(&p8022_list_lock, flags);
if (find_8022_client(type))
goto out;
proto = kmalloc(sizeof(*proto), GFP_ATOMIC);
@@ -99,7 +105,8 @@
p8022_list = proto;
llc_register_sap(type, p8022_rcv);
}
-out: return proto;
+out: write_unlock_irqrestore(&p8022_list_lock, flags);
+ return proto;
}
void unregister_8022_client(unsigned char type)
@@ -107,8 +114,7 @@
struct datalink_proto *tmp, **clients = &p8022_list;
unsigned long flags;
- save_flags(flags);
- cli();
+ write_lock_irqsave(&p8022_list_lock, flags);
while (*clients) {
tmp = *clients;
if (tmp->type[0] == type) {
@@ -119,7 +125,7 @@
}
clients = &tmp->next;
}
- restore_flags(flags);
+ write_unlock_irqrestore(&p8022_list_lock, flags);
}
EXPORT_SYMBOL(register_8022_client);
diff -urdN linux/net/802/psnap.c linux/net/802/psnap.c
--- linux/net/802/psnap.c 2002-07-22 13:32:58.000000000 +0000
+++ linux/net/802/psnap.c 2002-07-22 19:52:11.000000000 +0000
@@ -22,6 +22,7 @@
static struct datalink_proto *snap_list = NULL;
static struct datalink_proto *snap_dl = NULL; /* 802.2 DL for SNAP */
+static rwlock_t snap_list_lock = RW_LOCK_UNLOCKED;
/*
* Find a snap client by matching the 5 bytes.
@@ -52,9 +53,12 @@
struct datalink_proto *proto;
+ read_lock(&snap_list_lock);
proto = find_snap_client(skb->h.raw);
if (proto != NULL)
{
+ int err;
+
/*
* Pass the frame on.
*/
@@ -65,8 +69,11 @@
if (psnap_packet_type.type == 0)
psnap_packet_type.type=htons(ETH_P_SNAP);
- return proto->rcvfunc(skb, dev, &psnap_packet_type);
+ err = proto->rcvfunc(skb, dev, &psnap_packet_type);
+ read_unlock(&snap_list_lock);
+ return err;
}
+ read_unlock(&snap_list_lock);
skb->sk = NULL;
kfree_skb(skb);
return 0;
@@ -104,10 +111,12 @@
struct datalink_proto *register_snap_client(unsigned char *desc, int (*rcvfunc)(struct sk_buff *, struct net_device *, struct packet_type *))
{
- struct datalink_proto *proto;
+ struct datalink_proto *proto = NULL;
+ unsigned long flags;
+ write_lock_irqsave(&snap_list_lock, flags);
if (find_snap_client(desc) != NULL)
- return NULL;
+ goto out;
proto = (struct datalink_proto *) kmalloc(sizeof(*proto), GFP_ATOMIC);
if (proto != NULL)
@@ -121,7 +130,7 @@
proto->next = snap_list;
snap_list = proto;
}
-
+out: write_unlock_irqrestore(&snap_list_lock, flags);
return proto;
}
@@ -135,8 +144,7 @@
struct datalink_proto *tmp;
unsigned long flags;
- save_flags(flags);
- cli();
+ write_lock_irqsave(&snap_list_lock, flags);
while ((tmp = *clients) != NULL)
{
@@ -150,5 +158,5 @@
clients = &tmp->next;
}
- restore_flags(flags);
+ write_unlock_irqrestore(&snap_list_lock, flags);
}
Hi Dave,
during my work on removing cli/sti from these two files I found that
removing psnap causes kernel to crash because of it does not unregister
from 802.2 layer.
While I was on it, I also added MOD_INC_USE_COUNT/MOD_DEC_USE_COUNT
into both 802.2 and SNAP. It is not strictly required as all callers
should have reference by name to register_*_client function, but I do not
think that MOD_*_USE_COUNT will slow code down, and now it is directly
visible how many snap and 802.2 clients live on system. And I'm sure
that somebody could through inter_module_get arrange some call path
so that we could unload p8022/psnap while still having some clients
registered.
Thanks,
Petr Vandrovec
[email protected]
diff linux-2.5.27-c683/net/802/p8022.c.orig linux-2.5.27-c683/net/802/p8022.c
--- linux-2.5.27-c683/net/802/p8022.c.orig 2002-07-22 21:32:20.000000000 +0200
+++ linux-2.5.27-c683/net/802/p8022.c 2002-07-22 23:12:43.000000000 +0200
@@ -95,6 +95,7 @@
goto out;
proto = kmalloc(sizeof(*proto), GFP_ATOMIC);
if (proto) {
+ MOD_INC_USE_COUNT;
proto->type[0] = type;
proto->type_len = 1;
proto->rcvfunc = rcvfunc;
@@ -121,6 +122,7 @@
*clients = tmp->next;
kfree(tmp);
llc_unregister_sap(type);
+ MOD_DEC_USE_COUNT;
break;
}
clients = &tmp->next;
diff linux-2.5.27-c683/net/802/psnap.c.orig linux-2.5.27-c683/net/802/psnap.c
--- linux-2.5.27-c683/net/802/psnap.c.orig 2002-07-22 21:52:11.000000000 +0200
+++ linux-2.5.27-c683/net/802/psnap.c 2002-07-22 23:14:34.000000000 +0200
@@ -103,7 +103,14 @@
printk("SNAP - unable to register with 802.2\n");
return 0;
}
+
+static void __exit snap_exit(void)
+{
+ unregister_8022_client(0xAA);
+}
+
module_init(snap_init);
+module_exit(snap_exit);
/*
* Register SNAP clients. We don't yet use this for IP.
@@ -121,6 +128,7 @@
proto = (struct datalink_proto *) kmalloc(sizeof(*proto), GFP_ATOMIC);
if (proto != NULL)
{
+ MOD_INC_USE_COUNT;
memcpy(proto->type, desc,5);
proto->type_len = 5;
proto->rcvfunc = rcvfunc;
@@ -152,6 +160,7 @@
{
*clients = tmp->next;
kfree(tmp);
+ MOD_DEC_USE_COUNT;
break;
}
else
These patches don't make any sense.
You aren't blocking against other things that cli/sti used
to disable, namely timers and the generic input packet processing
engine.
There is no way these changes are a correct replacement for cli/sti.
This goes for your IPX changes too which I ask that you had pass
through Arnaldo de Melo in the future as he has done a lot of work
in this area.
On 22 Jul 02 at 19:07, David S. Miller wrote:
>
> These patches don't make any sense.
>
> You aren't blocking against other things that cli/sti used
> to disable, namely timers and the generic input packet processing
> engine.
>
> There is no way these changes are a correct replacement for cli/sti.
> This goes for your IPX changes too which I ask that you had pass
> through Arnaldo de Melo in the future as he has done a lot of work
> in this area.
Why? I'm definitely sure that IPX patches are correct: only
place which accesses spx_family_ops variable is ipx_create.
If we have SPX sockets created when we call ipx_unregister_spx, we
have much worse problem, because of regardless of any cli/sti, we are
going to release af_spx memory very soon, and cli does not force us to
close all SPX sockets, does it?
As for p8022/psnap changes, yes, I missed datalink_header locking.
But because of IPX uses dl->datalink_header() happilly from process
context without any locking, I thought that users of proto structure
returned from register_* are responsible for making sure that they'll
not use it anymore when they call unregister_*. Adding (any) lock
into *_datalink_header will not help unless you will call find_*_client
in *_datalink_header after obtaining lock to revalidate pointer you
received from caller. Which is obviously wrong, I'd say. And ipx
requires you to down all interfaces/close all sockets before it will
call unregister_*_client, so IPX is safe here (at least as it was always;
if kernel can send packets to downed interface, it is completely another
problem).
Thanks for explanation,
Petr Vandrovec
[email protected]