* [PATCH] : ir243_ttp_sock_races-2.diff
@ 2001-11-08 3:15 Jean Tourrilhes
0 siblings, 0 replies; only message in thread
From: Jean Tourrilhes @ 2001-11-08 3:15 UTC (permalink / raw)
To: Linus Torvalds, Alan Cox, Linux kernel mailing list, linux-irda
Hi,
Please apply....
Jean
ir243_ttp_sock_races-2.diff :
---------------------------
o [CRITICA] Avoid race condition in TSAP closure
o [CRITICA] Avoid race condition in socket closure
o [CRITICA] Avoid various race conditions in IrNET
o [CORRECT] Remove potential skb leaks
o [CORRECT] Remove IrNET discovery log leak
o [CORRECT] Don't export inline function (irda_lock)
o [FEATURE] Use skb_orphan() instead of skb_clone() + kfree_skb()
o [FEATURE] cleanups, comments
o [FEATURE] Reduce verbosity of the stack
diff -u -p linux/include/net/irda/irmod.d2.h linux/include/net/irda/irmod.h
--- linux/include/net/irda/irmod.d2.h Wed Nov 7 13:21:31 2001
+++ linux/include/net/irda/irmod.h Wed Nov 7 13:57:41 2001
@@ -10,6 +10,7 @@
* Modified by: Dag Brattli <dagb@cs.uit.no>
*
* Copyright (c) 1998-2000 Dag Brattli, All Rights Reserved.
+ * Copyright (c) 2000-2001 Jean Tourrilhes <jt@hpl.hp.com>
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
@@ -25,108 +26,19 @@
#ifndef IRMOD_H
#define IRMOD_H
-#include <linux/skbuff.h>
-#include <linux/miscdevice.h>
+#include <net/irda/irda.h> /* Notify stuff */
-#include <net/irda/irqueue.h>
+/* Nothing much here anymore - Maybe this header should be merged in
+ * another header like net/irda/irda.h... - Jean II */
-#define IRMGR_IOC_MAGIC 'm'
-#define IRMGR_IOCTNPC _IO(IRMGR_IOC_MAGIC, 1)
-#define IRMGR_IOC_MAXNR 1
-
-/*
- * Events that we pass to the user space manager
- */
-typedef enum {
- EVENT_DEVICE_DISCOVERED = 0,
- EVENT_REQUEST_MODULE,
- EVENT_IRLAN_START,
- EVENT_IRLAN_STOP,
- EVENT_IRLPT_START, /* Obsolete */
- EVENT_IRLPT_STOP, /* Obsolete */
- EVENT_IROBEX_START, /* Obsolete */
- EVENT_IROBEX_STOP, /* Obsolete */
- EVENT_IRDA_STOP,
- EVENT_NEED_PROCESS_CONTEXT,
-} IRMGR_EVENT;
-
-/*
- * Event information passed to the IrManager daemon process
- */
-struct irmanager_event {
- IRMGR_EVENT event;
- char devname[10];
- char info[32];
- int service;
- __u32 saddr;
- __u32 daddr;
-};
-
-typedef void (*TODO_CALLBACK)( void *self, __u32 param);
-
-/*
- * Same as irmanager_event but this one can be queued and inclueds some
- * addtional information
- */
-struct irda_event {
- irda_queue_t q; /* Must be first */
-
- struct irmanager_event event;
-};
-
-/*
- * Funtions with needs to be called with a process context
- */
-struct irda_todo {
- irda_queue_t q; /* Must be first */
-
- void *self;
- TODO_CALLBACK callback;
- __u32 param;
-};
-
-/*
- * Main structure for the IrDA device (not much here :-)
- */
-struct irda_cb {
- struct miscdevice dev;
- wait_queue_head_t wait_queue;
-
- int in_use;
-
- irda_queue_t *event_queue; /* Events queued for the irmanager */
- irda_queue_t *todo_queue; /* Todo list */
-};
-
-int irmod_init_module(void);
-void irmod_cleanup_module(void);
-
-/*
- * Function irda_lock (lock)
- *
- * Lock variable. Returns false if the lock is already set.
- *
- */
-static inline int irda_lock(int *lock)
-{
- if (test_and_set_bit( 0, (void *) lock)) {
- IRDA_DEBUG(3, __FUNCTION__
- "(), Trying to lock, already locked variable!\n");
- return FALSE;
- }
- return TRUE;
-}
-
-inline int irda_unlock(int *lock);
+/* Locking wrapper - Note the inverted logic on irda_lock().
+ * Those function basically return false if the lock is already in the
+ * position you want to set it. - Jean II */
+#define irda_lock(lock) (! test_and_set_bit(0, (void *) (lock)))
+#define irda_unlock(lock) (test_and_clear_bit(0, (void *) (lock)))
+/* Zero the notify structure */
void irda_notify_init(notify_t *notify);
-
-void irda_execute_as_process(void *self, TODO_CALLBACK callback, __u32 param);
-void irmanager_notify(struct irmanager_event *event);
-
-extern void irda_proc_modcount(struct inode *, int);
-void irda_mod_inc_use_count(void);
-void irda_mod_dec_use_count(void);
#endif /* IRMOD_H */
diff -u -p linux/net/irda/irsyms.d2.c linux/net/irda/irsyms.c
--- linux/net/irda/irsyms.d2.c Wed Nov 7 13:16:00 2001
+++ linux/net/irda/irsyms.c Wed Nov 7 13:42:19 2001
@@ -10,6 +10,7 @@
* Modified by: Dag Brattli <dagb@cs.uit.no>
*
* Copyright (c) 1997, 1999-2000 Dag Brattli, All Rights Reserved.
+ * Copyright (c) 2000-2001 Jean Tourrilhes <jt@hpl.hp.com>
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
@@ -79,7 +80,6 @@ EXPORT_SYMBOL(irttp_dup);
EXPORT_SYMBOL(irda_debug);
#endif
EXPORT_SYMBOL(irda_notify_init);
-EXPORT_SYMBOL(irda_lock);
#ifdef CONFIG_PROC_FS
EXPORT_SYMBOL(proc_irda);
#endif
@@ -217,21 +217,6 @@ static void __exit irda_cleanup(void)
/* Remove middle layer */
irlmp_cleanup();
-}
-
-/*
- * Function irda_unlock (lock)
- *
- * Unlock variable. Returns false if lock is already unlocked
- *
- */
-inline int irda_unlock(int *lock)
-{
- if (!test_and_clear_bit(0, (void *) lock)) {
- printk("Trying to unlock already unlocked variable!\n");
- return FALSE;
- }
- return TRUE;
}
/*
diff -u -p linux/net/irda/irttp.d2.c linux/net/irda/irttp.c
--- linux/net/irda/irttp.d2.c Tue Oct 30 18:14:58 2001
+++ linux/net/irda/irttp.c Wed Nov 7 13:33:09 2001
@@ -11,6 +11,7 @@
*
* Copyright (c) 1998-2000 Dag Brattli <dagb@cs.uit.no>,
* All Rights Reserved.
+ * Copyright (c) 2000-2001 Jean Tourrilhes <jt@hpl.hp.com>
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
@@ -223,6 +224,11 @@ static void __irttp_close_tsap(struct ts
del_timer(&self->todo_timer);
+ /* This one won't be cleaned up if we are diconnect_pend + close_pend
+ * and we receive a disconnect_indication */
+ if (self->disconnect_skb)
+ dev_kfree_skb(self->disconnect_skb);
+
self->connected = FALSE;
self->magic = ~TTP_TSAP_MAGIC;
@@ -235,6 +241,9 @@ static void __irttp_close_tsap(struct ts
* Remove TSAP from list of all TSAPs and then deallocate all resources
* associated with this TSAP
*
+ * Note : because we *free* the tsap structure, it is the responsability
+ * of the caller to make sure we are called only once and to deal with
+ * possible race conditions. - Jean II
*/
int irttp_close_tsap(struct tsap_cb *self)
{
@@ -248,8 +257,8 @@ int irttp_close_tsap(struct tsap_cb *sel
/* Make sure tsap has been disconnected */
if (self->connected) {
/* Check if disconnect is not pending */
- if (!self->disconnect_pend) {
- IRDA_DEBUG(0, __FUNCTION__ "(), TSAP still connected!\n");
+ if (!test_bit(0, &self->disconnect_pend)) {
+ WARNING(__FUNCTION__ "(), TSAP still connected!\n");
irttp_disconnect_request(self, NULL, P_NORMAL);
}
self->close_pend = TRUE;
@@ -407,6 +416,7 @@ static void irttp_run_tx_queue(struct ts
unsigned long flags;
int n;
+ /* Get exclusive access to the tx queue, otherwise don't touch it */
if (irda_lock(&self->tx_queue_lock) == FALSE)
return;
@@ -473,27 +483,17 @@ static void irttp_run_tx_queue(struct ts
* close the socket, we are dead !
* Jean II */
if (skb->sk != NULL) {
- struct sk_buff *tx_skb;
-
/* IrSOCK application, IrOBEX, ... */
IRDA_DEBUG(4, __FUNCTION__ "() : Detaching SKB from socket.\n");
- /* Note : still looking for a more efficient way
- * to do that - Jean II */
- /* Get another skb on the same buffer, but without
- * a reference to the socket (skb->sk = NULL) */
- tx_skb = skb_clone(skb, GFP_ATOMIC);
- if (tx_skb != NULL) {
- /* Release the skb associated with the
- * socket, and use the new skb insted */
- kfree_skb(skb);
- skb = tx_skb;
- }
+ /* That's the right way to do it - Jean II */
+ skb_orphan(skb);
} else {
/* IrCOMM over IrTTP, IrLAN, ... */
IRDA_DEBUG(4, __FUNCTION__ "() : Got SKB not attached to a socket.\n");
}
+ /* Pass the skb to IrLMP - done */
irlmp_data_request(self->lsap, skb);
self->stats.tx_packets++;
@@ -1105,18 +1105,23 @@ int irttp_disconnect_request(struct tsap
/* Already disconnected? */
if (!self->connected) {
IRDA_DEBUG(4, __FUNCTION__ "(), already disconnected!\n");
+ if (userdata)
+ dev_kfree_skb(userdata);
return -1;
}
- /* Disconnect already pending? */
- if (self->disconnect_pend) {
- IRDA_DEBUG(1, __FUNCTION__ "(), disconnect already pending\n");
- if (userdata) {
+ /* Disconnect already pending ?
+ * We need to use an atomic operation to prevent reentry. This
+ * function may be called from various context, like user, timer
+ * for following a disconnect_indication() (i.e. net_bh).
+ * Jean II */
+ if(test_and_set_bit(0, &self->disconnect_pend)) {
+ IRDA_DEBUG(0, __FUNCTION__ "(), disconnect already pending\n");
+ if (userdata)
dev_kfree_skb(userdata);
- }
/* Try to make some progress */
- irttp_run_rx_queue(self);
+ irttp_run_tx_queue(self);
return -1;
}
@@ -1125,25 +1130,20 @@ int irttp_disconnect_request(struct tsap
*/
if (skb_queue_len(&self->tx_queue) > 0) {
if (priority == P_HIGH) {
- IRDA_DEBUG(1, __FUNCTION__ "High priority!!()\n" );
-
/*
* No need to send the queued data, if we are
* disconnecting right now since the data will
* not have any usable connection to be sent on
*/
+ IRDA_DEBUG(1, __FUNCTION__ "High priority!!()\n" );
irttp_flush_queues(self);
} else if (priority == P_NORMAL) {
/*
- * Must delay disconnect til after all data segments
- * have been sent an the tx_queue is empty
+ * Must delay disconnect until after all data segments
+ * have been sent and the tx_queue is empty
*/
- if (userdata)
- self->disconnect_skb = userdata;
- else
- self->disconnect_skb = NULL;
-
- self->disconnect_pend = TRUE;
+ /* We'll reuse this one later for the disconnect */
+ self->disconnect_skb = userdata; /* May be NULL */
irttp_run_tx_queue(self);
@@ -1152,9 +1152,8 @@ int irttp_disconnect_request(struct tsap
}
}
IRDA_DEBUG(1, __FUNCTION__ "(), Disconnecting ...\n");
-
self->connected = FALSE;
-
+
if (!userdata) {
skb = dev_alloc_skb(64);
if (!skb)
@@ -1169,6 +1168,9 @@ int irttp_disconnect_request(struct tsap
}
ret = irlmp_disconnect_request(self->lsap, userdata);
+ /* The disconnect is no longer pending */
+ clear_bit(0, &self->disconnect_pend); /* FALSE */
+
return ret;
}
@@ -1190,19 +1192,27 @@ void irttp_disconnect_indication(void *i
ASSERT(self != NULL, return;);
ASSERT(self->magic == TTP_TSAP_MAGIC, return;);
+ /* Prevent higher layer to send more data */
self->connected = FALSE;
/* Check if client has already tried to close the TSAP */
if (self->close_pend) {
+ /* In this case, the higher layer is probably gone. Don't
+ * bother it and clean up the remains - Jean II */
+ if (skb)
+ dev_kfree_skb(skb);
irttp_close_tsap(self);
return;
}
+ /* If we are here, we assume that is the higher layer is still
+ * waiting for the disconnect notification and able to process it,
+ * even if he tried to disconnect. Otherwise, it would have already
+ * attempted to close the tsap and self->close_pend would be TRUE.
+ * Jean II */
+
/* No need to notify the client if has already tried to disconnect */
- if (self->disconnect_pend)
- return;
-
- if (self->notify.disconnect_indication)
+ if(self->notify.disconnect_indication)
self->notify.disconnect_indication(self->notify.instance, self,
reason, skb);
else
@@ -1222,7 +1232,7 @@ void irttp_do_data_indication(struct tsa
int err;
/* Check if client has already tried to close the TSAP */
- if (self->close_pend || self->disconnect_pend) {
+ if (self->close_pend) {
dev_kfree_skb(skb);
return;
}
@@ -1263,6 +1273,7 @@ void irttp_run_rx_queue(struct tsap_cb *
IRDA_DEBUG(2, __FUNCTION__ "() send=%d,avail=%d,remote=%d\n",
self->send_credit, self->avail_credit, self->remote_credit);
+ /* Get exclusive access to the rx queue, otherwise don't touch it */
if (irda_lock(&self->rx_queue_lock) == FALSE)
return;
@@ -1500,7 +1511,7 @@ static int irttp_param_max_sdu_size(void
else
self->tx_max_sdu_size = param->pv.i;
- IRDA_DEBUG(0, __FUNCTION__ "(), MaxSduSize=%d\n", param->pv.i);
+ IRDA_DEBUG(1, __FUNCTION__ "(), MaxSduSize=%d\n", param->pv.i);
return 0;
}
@@ -1530,18 +1541,16 @@ static void irttp_todo_expired(unsigned
}
/* Check if time for disconnect */
- if (self->disconnect_pend) {
+ if (test_bit(0, &self->disconnect_pend)) {
/* Check if it's possible to disconnect yet */
if (skb_queue_empty(&self->tx_queue)) {
-
/* Make sure disconnect is not pending anymore */
- self->disconnect_pend = FALSE;
- if (self->disconnect_skb) {
- irttp_disconnect_request(
- self, self->disconnect_skb, P_NORMAL);
- self->disconnect_skb = NULL;
- } else
- irttp_disconnect_request(self, NULL, P_NORMAL);
+ clear_bit(0, &self->disconnect_pend); /* FALSE */
+
+ /* Note : self->disconnect_skb may be NULL */
+ irttp_disconnect_request(self, self->disconnect_skb,
+ P_NORMAL);
+ self->disconnect_skb = NULL;
} else {
/* Try again later */
irttp_start_todo_timer(self, 1*HZ);
diff -u -p linux/net/irda/af_irda.d2.c linux/net/irda/af_irda.c
--- linux/net/irda/af_irda.d2.c Fri Nov 2 11:09:12 2001
+++ linux/net/irda/af_irda.c Mon Nov 5 18:40:11 2001
@@ -11,7 +11,7 @@
* Sources: af_netroom.c, af_ax25.c, af_rose.c, af_x25.c etc.
*
* Copyright (c) 1999 Dag Brattli <dagb@cs.uit.no>
- * Copyright (c) 1999 Jean Tourrilhes <jt@hpl.hp.com>
+ * Copyright (c) 1999-2001 Jean Tourrilhes <jt@hpl.hp.com>
* All Rights Reserved.
*
* This program is free software; you can redistribute it and/or
@@ -134,33 +134,41 @@ static void irda_disconnect_indication(v
IRDA_DEBUG(2, __FUNCTION__ "(%p)\n", self);
+ /* Don't care about it, but let's not leak it */
+ if(skb)
+ dev_kfree_skb(skb);
+
sk = self->sk;
if (sk == NULL)
return;
- sk->state = TCP_CLOSE;
- sk->err = ECONNRESET;
- sk->shutdown |= SEND_SHUTDOWN;
- if (!sk->dead) {
+ /* Prevent race conditions with irda_release() and irda_shutdown() */
+ if ((!sk->dead) && (sk->state != TCP_CLOSE)) {
+ sk->state = TCP_CLOSE;
+ sk->err = ECONNRESET;
+ sk->shutdown |= SEND_SHUTDOWN;
+
sk->state_change(sk);
- sk->dead = 1;
- }
+ sk->dead = 1; /* Uh-oh... Should use sock_orphan ? */
- /* Close our TSAP.
- * If we leave it open, IrLMP put it back into the list of
- * unconnected LSAPs. The problem is that any incoming request
- * can then be matched to this socket (and it will be, because
- * it is at the head of the list). This would prevent any
- * listening socket waiting on the same TSAP to get those requests.
- * Some apps forget to close sockets, or hang to it a bit too long,
- * so we may stay in this dead state long enough to be noticed...
- * Note : all socket function do check sk->state, so we are safe...
- * Jean II
- */
- if (self->tsap) {
- irttp_close_tsap(self->tsap);
- self->tsap = NULL;
- }
+ /* Close our TSAP.
+ * If we leave it open, IrLMP put it back into the list of
+ * unconnected LSAPs. The problem is that any incoming request
+ * can then be matched to this socket (and it will be, because
+ * it is at the head of the list). This would prevent any
+ * listening socket waiting on the same TSAP to get those
+ * requests. Some apps forget to close sockets, or hang to it
+ * a bit too long, so we may stay in this dead state long
+ * enough to be noticed...
+ * Note : all socket function do check sk->state, so we are
+ * safe...
+ * Jean II
+ */
+ if (self->tsap) {
+ irttp_close_tsap(self->tsap);
+ self->tsap = NULL;
+ }
+ }
/* Note : once we are there, there is not much you want to do
* with the socket anymore, apart from closing it.
@@ -222,7 +230,8 @@ static void irda_connect_confirm(void *i
self->max_data_size);
memcpy(&self->qos_tx, qos, sizeof(struct qos_info));
- kfree_skb(skb);
+ dev_kfree_skb(skb);
+ // Should be ??? skb_queue_tail(&sk->receive_queue, skb);
/* We are now connected! */
sk->state = TCP_ESTABLISHED;
@@ -1205,7 +1214,7 @@ static int irda_release(struct socket *s
sk->protinfo.irda = NULL;
sock_orphan(sk);
- sock->sk = NULL;
+ sock->sk = NULL;
/* Purge queues (see sock_init_data()) */
skb_queue_purge(&sk->receive_queue);
diff -u -p linux/net/irda/iriap.d2.c linux/net/irda/iriap.c
--- linux/net/irda/iriap.d2.c Fri Nov 2 11:14:13 2001
+++ linux/net/irda/iriap.c Mon Nov 5 18:41:12 2001
@@ -11,6 +11,7 @@
*
* Copyright (c) 1998-1999 Dag Brattli <dagb@cs.uit.no>,
* All Rights Reserved.
+ * Copyright (c) 2000-2001 Jean Tourrilhes <jt@hpl.hp.com>
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
@@ -773,7 +774,7 @@ static void iriap_connect_indication(voi
{
struct iriap_cb *self, *new;
- IRDA_DEBUG(0, __FUNCTION__ "()\n");
+ IRDA_DEBUG(1, __FUNCTION__ "()\n");
self = (struct iriap_cb *) instance;
diff -u -p linux/net/irda/irnet/irnet.d2.h linux/net/irda/irnet/irnet.h
--- linux/net/irda/irnet/irnet.d2.h Wed Oct 31 16:09:19 2001
+++ linux/net/irda/irnet/irnet.h Wed Nov 7 14:02:34 2001
@@ -126,17 +126,17 @@
* History :
* -------
*
- * v1 - 15/5/00 - Jean II
+ * v1 - 15.5.00 - Jean II
* o Basic IrNET (hook to ppp_generic & IrTTP - incl. multipoint)
* o control channel on /dev/irnet (set name/address)
* o event channel on /dev/irnet (for user space daemon)
*
- * v2 - 5/6/00 - Jean II
+ * v2 - 5.6.00 - Jean II
* o Enable DROP_NOT_READY to avoid PPP timeouts & other weirdness...
* o Add DISCONNECT_TO event and rename DISCONNECT_FROM.
* o Set official device number alloaction on /dev/irnet
*
- * v3 - 30/8/00 - Jean II
+ * v3 - 30.8.00 - Jean II
* o Update to latest Linux-IrDA changes :
* - queue_t => irda_queue_t
* o Update to ppp-2.4.0 :
@@ -148,17 +148,17 @@
* another multilink bug (darn !)
* o Remove LINKNAME_IOCTL cruft
*
- * v3b - 31/8/00 - Jean II
+ * v3b - 31.8.00 - Jean II
* o Dump discovery log at event channel startup
*
- * v4 - 28/9/00 - Jean II
+ * v4 - 28.9.00 - Jean II
* o Fix interaction between poll/select and dump discovery log
* o Add IRNET_BLOCKED_LINK event (depend on new IrDA-Linux patch)
* o Add IRNET_NOANSWER_FROM event (mostly to help support)
* o Release flow control in disconnect_indication
* o Block packets while connecting (speed up connections)
*
- * v5 - 11/01/01 - Jean II
+ * v5 - 11.01.01 - Jean II
* o Init self->max_header_size, just in case...
* o Set up ap->chan.hdrlen, to get zero copy on tx side working.
* o avoid tx->ttp->flow->ppp->tx->... loop, by checking flow state
@@ -169,7 +169,7 @@
* o Declare hashbin HB_NOLOCK instead of HB_LOCAL to avoid
* disabling and enabling irq twice
*
- * v6 - 31/05/01 - Jean II
+ * v6 - 31.05.01 - Jean II
* o Print source address in Found, Discovery, Expiry & Request events
* o Print requested source address in /proc/net/irnet
* o Change control channel input. Allow multiple commands in one line.
@@ -186,12 +186,19 @@
* o Add ttp_connect flag to prevent rentry on the connect procedure
* o Test and fixups to eliminate side effects of retries
*
- * v7 - 22/08/01 - Jean II
+ * v7 - 22.08.01 - Jean II
* o Cleanup : Change "saddr = 0x0" to "saddr = DEV_ADDR_ANY"
* o Fix bug in BLOCK_WHEN_CONNECT introduced in v6 : due to the
* asynchronous IAS query, self->tsap is NULL when PPP send the
* first packet. This was preventing "connect-delay 0" to work.
* Change the test in ppp_irnet_send() to self->ttp_connect.
+ *
+ * v8 - 1.11.01 - Jean II
+ * o Tighten the use of self->ttp_connect and self->ttp_open to
+ * prevent various race conditions.
+ * o Avoid leaking discovery log and skb
+ * o Replace "self" with "server" in irnet_connect_indication() to
+ * better detect cut'n'paste error ;-)
*/
/***************************** INCLUDES *****************************/
@@ -204,6 +211,7 @@
#include <linux/proc_fs.h>
#include <linux/devfs_fs_kernel.h>
#include <linux/netdevice.h>
+#include <linux/miscdevice.h>
#include <linux/poll.h>
#include <linux/config.h>
#include <linux/ctype.h> /* isspace() */
diff -u -p linux/net/irda/irnet/irnet_irda.d2.c linux/net/irda/irnet/irnet_irda.c
--- linux/net/irda/irnet/irnet_irda.d2.c Wed Oct 31 16:00:53 2001
+++ linux/net/irda/irnet/irnet_irda.c Wed Nov 7 10:48:58 2001
@@ -272,7 +272,7 @@ irnet_connect_tsap(irnet_socket * self)
err = irnet_open_tsap(self);
if(err != 0)
{
- self->ttp_connect = 0;
+ clear_bit(0, &self->ttp_connect);
DERROR(IRDA_SR_ERROR, "connect aborted!\n");
return(err);
}
@@ -283,7 +283,7 @@ irnet_connect_tsap(irnet_socket * self)
self->max_sdu_size_rx, NULL);
if(err != 0)
{
- self->ttp_connect = 0;
+ clear_bit(0, &self->ttp_connect);
DERROR(IRDA_SR_ERROR, "connect aborted!\n");
return(err);
}
@@ -377,7 +377,7 @@ irnet_discover_daddr_and_lsap_sel(irnet_
if(self->discoveries == NULL)
{
self->disco_number = -1;
- self->ttp_connect = 0;
+ clear_bit(0, &self->ttp_connect);
DRETURN(-ENETUNREACH, IRDA_SR_INFO, "No Cachelog...\n");
}
DEBUG(IRDA_SR_INFO, "Got the log (0x%X), size is %d\n",
@@ -399,7 +399,7 @@ irnet_discover_daddr_and_lsap_sel(irnet_
kfree(self->discoveries);
self->discoveries = NULL;
- self->ttp_connect = 0;
+ clear_bit(0, &self->ttp_connect);
DRETURN(-ENETUNREACH, IRDA_SR_INFO, "Cachelog empty...\n");
}
@@ -518,12 +518,12 @@ irda_irnet_connect(irnet_socket * self)
DENTER(IRDA_SOCK_TRACE, "(self=0x%X)\n", (unsigned int) self);
- /* Check if we have opened a local TSAP :
- * If we have already opened a TSAP, it means that either we are already
- * connected or in the process of doing so... */
- if(self->ttp_connect)
+ /* Check if we are already trying to connect.
+ * Because irda_irnet_connect() can be called directly by pppd plus
+ * packet retries in ppp_generic and connect may take time, plus we may
+ * race with irnet_connect_indication(), we need to be careful there... */
+ if(test_and_set_bit(0, &self->ttp_connect))
DRETURN(-EBUSY, IRDA_SOCK_INFO, "Already connecting...\n");
- self->ttp_connect = 1;
if((self->iriap != NULL) || (self->tsap != NULL))
DERROR(IRDA_SOCK_ERROR, "Socket not cleaned up...\n");
@@ -579,6 +579,7 @@ irda_irnet_connect(irnet_socket * self)
*
* Destroy irnet instance
*
+ * Note : this need to be called from a process context.
*/
void
irda_irnet_destroy(irnet_socket * self)
@@ -601,6 +602,23 @@ irda_irnet_destroy(irnet_socket * self)
DASSERT(entry == self, , IRDA_SOCK_ERROR, "Can't remove from hash.\n");
}
+ /* If we were connected, post a message */
+ if(test_bit(0, &self->ttp_open))
+ {
+ /* Note : as the disconnect comes from ppp_generic, the unit number
+ * doesn't exist anymore when we post the event, so we need to pass
+ * NULL as the first arg... */
+ irnet_post_event(NULL, IRNET_DISCONNECT_TO,
+ self->saddr, self->daddr, self->rname);
+ }
+
+ /* Prevent various IrDA callbacks from messing up things
+ * Need to be first */
+ clear_bit(0, &self->ttp_connect);
+
+ /* Prevent higher layer from accessing IrTTP */
+ clear_bit(0, &self->ttp_open);
+
/* Unregister with IrLMP */
irlmp_unregister_client(self->ckey);
@@ -611,19 +629,14 @@ irda_irnet_destroy(irnet_socket * self)
self->iriap = NULL;
}
- /* If we were connected, post a message */
- if(self->ttp_open)
+ /* Cleanup eventual discoveries from connection attempt */
+ if(self->discoveries != NULL)
{
- /* Note : as the disconnect comes from ppp_generic, the unit number
- * doesn't exist anymore when we post the event, so we need to pass
- * NULL as the first arg... */
- irnet_post_event(NULL, IRNET_DISCONNECT_TO,
- self->saddr, self->daddr, self->rname);
+ /* Cleanup our copy of the discovery log */
+ kfree(self->discoveries);
+ self->discoveries = NULL;
}
- /* Prevent higher layer from accessing IrTTP */
- self->ttp_open = 0;
-
/* Close our IrTTP connection */
if(self->tsap)
{
@@ -761,7 +774,7 @@ irnet_find_socket(irnet_socket * self)
while(new !=(irnet_socket *) NULL)
{
/* Is it available ? */
- if(!(new->ttp_open) && (new->rdaddr == DEV_ADDR_ANY) &&
+ if(!(test_bit(0, &new->ttp_open)) && (new->rdaddr == DEV_ADDR_ANY) &&
(new->rname[0] == '\0') && (new->ppp_open))
{
/* Yes !!! Get it.. */
@@ -788,17 +801,17 @@ irnet_find_socket(irnet_socket * self)
*
*/
static inline int
-irnet_connect_socket(irnet_socket * self,
+irnet_connect_socket(irnet_socket * server,
irnet_socket * new,
struct qos_info * qos,
__u32 max_sdu_size,
__u8 max_header_size)
{
- DENTER(IRDA_SERV_TRACE, "(self=0x%X, new=0x%X)\n",
- (unsigned int) self, (unsigned int) new);
+ DENTER(IRDA_SERV_TRACE, "(server=0x%X, new=0x%X)\n",
+ (unsigned int) server, (unsigned int) new);
/* Now attach up the new socket */
- new->tsap = irttp_dup(self->tsap, new);
+ new->tsap = irttp_dup(server->tsap, new);
DABORT(new->tsap == NULL, -1, IRDA_SERV_ERROR, "dup failed!\n");
/* Set up all the relevant parameters on the new socket */
@@ -817,17 +830,32 @@ irnet_connect_socket(irnet_socket * self
#endif /* STREAM_COMPAT */
/* Clean up the original one to keep it in listen state */
- self->tsap->dtsap_sel = self->tsap->lsap->dlsap_sel = LSAP_ANY;
- self->tsap->lsap->lsap_state = LSAP_DISCONNECTED;
+ server->tsap->dtsap_sel = server->tsap->lsap->dlsap_sel = LSAP_ANY;
+ server->tsap->lsap->lsap_state = LSAP_DISCONNECTED;
/* Send a connection response on the new socket */
irttp_connect_response(new->tsap, new->max_sdu_size_rx, NULL);
/* Allow PPP to send its junk over the new socket... */
- new->ttp_open = 1;
- new->ttp_connect = 0;
+ set_bit(0, &new->ttp_open);
+
+ /* Not connecting anymore, and clean up last possible remains
+ * of connection attempts on the socket */
+ clear_bit(0, &new->ttp_connect);
+ if(new->iriap)
+ {
+ iriap_close(new->iriap);
+ new->iriap = NULL;
+ }
+ if(new->discoveries != NULL)
+ {
+ kfree(new->discoveries);
+ new->discoveries = NULL;
+ }
+
#ifdef CONNECT_INDIC_KICK
- /* As currently we don't packets in ppp_irnet_send(), this is not needed...
+ /* As currently we don't block packets in ppp_irnet_send() while passive,
+ * this is not really needed...
* Also, not doing it give IrDA a chance to finish the setup properly
* before beeing swamped with packets... */
ppp_output_wakeup(&new->chan);
@@ -835,7 +863,7 @@ irnet_connect_socket(irnet_socket * self
/* Notify the control channel */
irnet_post_event(new, IRNET_CONNECT_FROM,
- new->saddr, new->daddr, self->rname);
+ new->saddr, new->daddr, server->rname);
DEXIT(IRDA_SERV_TRACE, "\n");
return 0;
@@ -1053,12 +1081,33 @@ irnet_disconnect_indication(void * insta
struct sk_buff *skb)
{
irnet_socket * self = (irnet_socket *) instance;
+ int test = 0;
DENTER(IRDA_TCB_TRACE, "(self=0x%X)\n", (unsigned int) self);
DASSERT(self != NULL, , IRDA_CB_ERROR, "Self is NULL !!!\n");
+ /* Don't care about it, but let's not leak it */
+ if(skb)
+ dev_kfree_skb(skb);
+
+ /* Prevent higher layer from accessing IrTTP */
+ test = test_and_clear_bit(0, &self->ttp_open);
+ /* Not connecting anymore...
+ * (note : TSAP is open, so IAP callbacks are no longer pending...) */
+ test |= test_and_clear_bit(0, &self->ttp_connect);
+
+ /* If both self->ttp_open and self->ttp_connect are NULL, it mean that we
+ * have a race condition with irda_irnet_destroy() or
+ * irnet_connect_indication(), so don't mess up tsap...
+ */
+ if(!test)
+ {
+ DERROR(IRDA_CB_ERROR, "Race condition detected...\n");
+ return;
+ }
+
/* If we were active, notify the control channel */
- if(self->ttp_open)
+ if(test_bit(0, &self->ttp_open))
irnet_post_event(self, IRNET_DISCONNECT_FROM,
self->saddr, self->daddr, self->rname);
else
@@ -1067,15 +1116,10 @@ irnet_disconnect_indication(void * insta
irnet_post_event(self, IRNET_NOANSWER_FROM,
self->saddr, self->daddr, self->rname);
- /* Prevent higher layer from accessing IrTTP */
- self->ttp_open = 0;
- self->ttp_connect = 0;
-
- /* Close our IrTTP connection */
+ /* Close our IrTTP connection, cleanup tsap */
if((self->tsap) && (self != &irnet_server.s))
{
DEBUG(IRDA_CB_INFO, "Closing our TTP connection.\n");
- irttp_disconnect_request(self->tsap, NULL, P_NORMAL);
irttp_close_tsap(self->tsap);
self->tsap = NULL;
@@ -1114,6 +1158,13 @@ irnet_connect_confirm(void * instance,
DENTER(IRDA_TCB_TRACE, "(self=0x%X)\n", (unsigned int) self);
+ /* Check if socket is closing down (via irda_irnet_destroy()) */
+ if(! test_bit(0, &self->ttp_connect))
+ {
+ DERROR(IRDA_CB_ERROR, "Socket no longer connecting. Ouch !\n");
+ return;
+ }
+
/* How much header space do we need to reserve */
self->max_header_size = max_header_size;
@@ -1129,8 +1180,8 @@ irnet_connect_confirm(void * instance,
self->saddr = irttp_get_saddr(self->tsap);
/* Allow higher layer to access IrTTP */
- self->ttp_connect = 0;
- self->ttp_open = 1;
+ set_bit(0, &self->ttp_open);
+ clear_bit(0, &self->ttp_connect); /* Not racy, IrDA traffic is serial */
/* Give a kick in the ass of ppp_generic so that he sends us some data */
ppp_output_wakeup(&self->chan);
@@ -1251,56 +1302,76 @@ irnet_connect_indication(void * instanc
__u8 max_header_size,
struct sk_buff *skb)
{
- irnet_socket * self = &irnet_server.s;
+ irnet_socket * server = &irnet_server.s;
irnet_socket * new = (irnet_socket *) NULL;
- DENTER(IRDA_TCB_TRACE, "(self=0x%X)\n", (unsigned int) self);
+ DENTER(IRDA_TCB_TRACE, "(server=0x%X)\n", (unsigned int) server);
DASSERT(instance == &irnet_server, , IRDA_CB_ERROR,
"Invalid instance (0x%X) !!!\n", (unsigned int) instance);
DASSERT(sap == irnet_server.s.tsap, , IRDA_CB_ERROR, "Invalid sap !!!\n");
/* Try to find the most appropriate IrNET socket */
- new = irnet_find_socket(self);
+ new = irnet_find_socket(server);
/* After all this hard work, do we have an socket ? */
if(new == (irnet_socket *) NULL)
{
DEXIT(IRDA_CB_INFO, ": No socket waiting for this connection.\n");
- irnet_disconnect_server(self, skb);
+ irnet_disconnect_server(server, skb);
return;
}
/* Is the socket already busy ? */
- if(new->ttp_open)
+ if(test_bit(0, &new->ttp_open))
{
DEXIT(IRDA_CB_INFO, ": Socket already connected.\n");
- irnet_disconnect_server(self, skb);
+ irnet_disconnect_server(server, skb);
return;
}
- /* Socket connecting */
- if(new->tsap != NULL)
- {
- /* The socket has sent a IrTTP connection request and is waiting for
- * a connection response (that may never come).
- * Now, the pain is that the socket has open a tsap and is waiting on it,
- * while the other end is trying to connect to it on another tsap.
- * Argh ! We will deal with that later...
+ /* Socket connecting ?
+ * Clear up flag : prevent irnet_disconnect_indication() to mess up tsap */
+ if(test_and_clear_bit(0, &new->ttp_connect))
+ {
+ /* The socket is trying to connect to the other end and may have sent
+ * a IrTTP connection request and is waiting for a connection response
+ * (that may never come).
+ * Now, the pain is that the socket may have opened a tsap and is
+ * waiting on it, while the other end is trying to connect to it on
+ * another tsap.
*/
DERROR(IRDA_CB_ERROR, "Socket already connecting. Ouch !\n");
#ifdef ALLOW_SIMULT_CONNECT
- /* Close the connection the new socket was attempting.
- * WARNING : This need more testing ! */
- irttp_close_tsap(new->tsap);
+ /* Cleanup the TSAP if necessary - IrIAP will be cleaned up later */
+ if(new->tsap != NULL)
+ {
+ /* Close the connection the new socket was attempting.
+ * This seems to be safe... */
+ irttp_close_tsap(new->tsap);
+ new->tsap = NULL;
+ }
/* Note : no return, fall through... */
#else /* ALLOW_SIMULT_CONNECT */
- irnet_disconnect_server(self, skb);
+ irnet_disconnect_server(server, skb);
return;
#endif /* ALLOW_SIMULT_CONNECT */
}
+ else
+ /* If socket is not connecting or connected, tsap should be NULL */
+ if(new->tsap != NULL)
+ {
+ /* If we are here, we are also in irnet_disconnect_indication(),
+ * and it's a nice race condition... On the other hand, we can't be
+ * in irda_irnet_destroy() otherwise we would not have found the
+ * socket in the hashbin. */
+ /* Better get out of here, otherwise we will mess up tsaps ! */
+ DERROR(IRDA_CB_ERROR, "Race condition detected, abort connect...\n");
+ irnet_disconnect_server(server, skb);
+ return;
+ }
/* So : at this point, we have a socket, and it is idle. Good ! */
- irnet_connect_socket(self, new, qos, max_sdu_size, max_header_size);
+ irnet_connect_socket(server, new, qos, max_sdu_size, max_header_size);
/* Check size of received packet */
if(skb->len > 0)
@@ -1349,24 +1420,25 @@ irnet_getvalue_confirm(int result,
DENTER(IRDA_OCB_TRACE, "(self=0x%X)\n", (unsigned int) self);
DASSERT(self != NULL, , IRDA_OCB_ERROR, "Self is NULL !!!\n");
- /* We probably don't need to make any more queries */
- iriap_close(self->iriap);
- self->iriap = NULL;
-
- /* Check if already connected (via irnet_connect_socket()) */
- if(self->ttp_open)
+ /* Check if already connected (via irnet_connect_socket())
+ * or socket is closing down (via irda_irnet_destroy()) */
+ if(! test_bit(0, &self->ttp_connect))
{
- DERROR(IRDA_OCB_ERROR, "Socket already connected. Ouch !\n");
+ DERROR(IRDA_OCB_ERROR, "Socket no longer connecting. Ouch !\n");
return;
}
+ /* We probably don't need to make any more queries */
+ iriap_close(self->iriap);
+ self->iriap = NULL;
+
/* Post process the IAS reply */
self->dtsap_sel = irnet_ias_to_tsap(self, result, value);
/* If error, just go out */
if(self->errno)
{
- self->ttp_connect = 0;
+ clear_bit(0, &self->ttp_connect);
DERROR(IRDA_OCB_ERROR, "IAS connect failed ! (0x%X)\n", self->errno);
return;
}
@@ -1412,6 +1484,14 @@ irnet_discovervalue_confirm(int result,
DENTER(IRDA_OCB_TRACE, "(self=0x%X)\n", (unsigned int) self);
DASSERT(self != NULL, , IRDA_OCB_ERROR, "Self is NULL !!!\n");
+ /* Check if already connected (via irnet_connect_socket())
+ * or socket is closing down (via irda_irnet_destroy()) */
+ if(! test_bit(0, &self->ttp_connect))
+ {
+ DERROR(IRDA_OCB_ERROR, "Socket no longer connecting. Ouch !\n");
+ return;
+ }
+
/* Post process the IAS reply */
dtsap_sel = irnet_ias_to_tsap(self, result, value);
@@ -1468,15 +1548,8 @@ irnet_discovervalue_confirm(int result,
if(self->daddr == DEV_ADDR_ANY)
{
self->daddr = DEV_ADDR_ANY;
- self->ttp_connect = 0;
+ clear_bit(0, &self->ttp_connect);
DEXIT(IRDA_OCB_TRACE, ": cannot discover IrNET in any device !!!\n");
- return;
- }
-
- /* Check if already connected (via irnet_connect_socket()) */
- if(self->ttp_open)
- {
- DERROR(IRDA_OCB_ERROR, "Socket already connected. Ouch !\n");
return;
}
diff -u -p linux/net/irda/irnet/irnet_ppp.d2.c linux/net/irda/irnet/irnet_ppp.c
--- linux/net/irda/irnet/irnet_ppp.d2.c Thu Nov 1 12:14:43 2001
+++ linux/net/irda/irnet/irnet_ppp.c Thu Nov 1 14:11:53 2001
@@ -850,7 +850,7 @@ ppp_irnet_send(struct ppp_channel * chan
DASSERT(self != NULL, 0, PPP_ERROR, "Self is NULL !!!\n");
/* Check if we are connected */
- if(self->ttp_open == 0)
+ if(!(test_bit(0, &self->ttp_open)))
{
#ifdef CONNECT_IN_SEND
/* Let's try to connect one more time... */
@@ -884,7 +884,7 @@ ppp_irnet_send(struct ppp_channel * chan
*/
#ifdef BLOCK_WHEN_CONNECT
/* If we are attempting to connect */
- if(self->ttp_connect)
+ if(test_bit(0, &self->ttp_connect))
{
/* Blocking packet, ppp_generic will retry later */
return 0;
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2001-11-08 3:19 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2001-11-08 3:15 [PATCH] : ir243_ttp_sock_races-2.diff Jean Tourrilhes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®