* [PATCH 2/5] caif: Fixes freeze on Link layer removal.
2011-05-22 21:18 [PATCH 1/5] caif: Bugfix add check NULL pointer before calling functions Sjur Brændeland
@ 2011-05-22 21:18 ` Sjur Brændeland
2011-05-23 0:13 ` David Miller
2011-05-22 21:18 ` [PATCH 3/5] caif: Fix freezes when running CAIF loopback device Sjur Brændeland
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Sjur Brændeland @ 2011-05-22 21:18 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: Sjur Brændeland
CAIF Socket layer - caif_socket.c:
- Plug mem-leak at reconnect.
- Always call disconnect to cleanup CAIF stack.
- Disconnect will always report success.
CAIF configuration layer - cfcnfg.c
- Disconnect must dismantle the caif stack correctly
- Protect against faulty removals (check on id zero)
CAIF mux layer - cfmuxl.c
- When inserting new service layer in the MUX remove
any old entries with the same ID.
- When removing CAIF Link layer, remove the associated
service layers before notifying service layers.
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
net/caif/caif_socket.c | 13 ++++-------
net/caif/cfcnfg.c | 44 ++++++++++++++++++------------------------
net/caif/cfmuxl.c | 49 +++++++++++++++++++++++++++++++++++++----------
3 files changed, 62 insertions(+), 44 deletions(-)
diff --git a/net/caif/caif_socket.c b/net/caif/caif_socket.c
index b840395..a986280 100644
--- a/net/caif/caif_socket.c
+++ b/net/caif/caif_socket.c
@@ -19,7 +19,7 @@
#include <linux/uaccess.h>
#include <linux/debugfs.h>
#include <linux/caif/caif_socket.h>
-#include <asm/atomic.h>
+#include <linux/atomic.h>
#include <net/sock.h>
#include <net/tcp_states.h>
#include <net/caif/caif_layer.h>
@@ -816,6 +816,7 @@ static int caif_connect(struct socket *sock, struct sockaddr *uaddr,
if (sk->sk_shutdown & SHUTDOWN_MASK) {
/* Allow re-connect after SHUTDOWN_IND */
caif_disconnect_client(sock_net(sk), &cf_sk->layer);
+ caif_free_client(&cf_sk->layer);
break;
}
/* No reconnect on a seqpacket socket */
@@ -926,7 +927,6 @@ static int caif_release(struct socket *sock)
{
struct sock *sk = sock->sk;
struct caifsock *cf_sk = container_of(sk, struct caifsock, sk);
- int res = 0;
if (!sk)
return 0;
@@ -953,10 +953,7 @@ static int caif_release(struct socket *sock)
sk->sk_state = CAIF_DISCONNECTED;
sk->sk_shutdown = SHUTDOWN_MASK;
- if (cf_sk->sk.sk_socket->state == SS_CONNECTED ||
- cf_sk->sk.sk_socket->state == SS_CONNECTING)
- res = caif_disconnect_client(sock_net(sk), &cf_sk->layer);
-
+ caif_disconnect_client(sock_net(sk), &cf_sk->layer);
cf_sk->sk.sk_socket->state = SS_DISCONNECTING;
wake_up_interruptible_poll(sk_sleep(sk), POLLERR|POLLHUP);
@@ -964,7 +961,7 @@ static int caif_release(struct socket *sock)
sk_stream_kill_queues(&cf_sk->sk);
release_sock(sk);
sock_put(sk);
- return res;
+ return 0;
}
/* Copied from af_unix.c:unix_poll(), added CAIF tx_flow handling */
@@ -1120,7 +1117,7 @@ static int caif_create(struct net *net, struct socket *sock, int protocol,
set_rx_flow_on(cf_sk);
/* Set default options on configuration */
- cf_sk->sk.sk_priority= CAIF_PRIO_NORMAL;
+ cf_sk->sk.sk_priority = CAIF_PRIO_NORMAL;
cf_sk->conn_req.link_selector = CAIF_LINK_LOW_LATENCY;
cf_sk->conn_req.protocol = protocol;
/* Increase the number of sockets created. */
diff --git a/net/caif/cfcnfg.c b/net/caif/cfcnfg.c
index 351c2ca..52fe33b 100644
--- a/net/caif/cfcnfg.c
+++ b/net/caif/cfcnfg.c
@@ -182,39 +182,26 @@ static int cfcnfg_get_id_from_ifi(struct cfcnfg *cnfg, int ifi)
int caif_disconnect_client(struct net *net, struct cflayer *adap_layer)
{
- u8 channel_id = 0;
- int ret = 0;
- struct cflayer *servl = NULL;
+ u8 channel_id;
struct cfcnfg *cfg = get_cfcnfg(net);
caif_assert(adap_layer != NULL);
-
- channel_id = adap_layer->id;
- if (adap_layer->dn == NULL || channel_id == 0) {
- pr_err("adap_layer->dn == NULL or adap_layer->id is 0\n");
- ret = -ENOTCONN;
- goto end;
- }
-
- servl = cfmuxl_remove_uplayer(cfg->mux, channel_id);
- if (servl == NULL) {
- pr_err("PROTOCOL ERROR - "
- "Error removing service_layer Channel_Id(%d)",
- channel_id);
- ret = -EINVAL;
- goto end;
- }
-
- ret = cfctrl_linkdown_req(cfg->ctrl, channel_id, adap_layer);
-
-end:
cfctrl_cancel_req(cfg->ctrl, adap_layer);
+ channel_id = adap_layer->id;
+ if (channel_id != 0) {
+ struct cflayer *servl;
+ servl = cfmuxl_remove_uplayer(cfg->mux, channel_id);
+ if (servl != NULL)
+ layer_set_up(servl, NULL);
+ } else
+ pr_debug("nothing to disconnect\n");
+ cfctrl_linkdown_req(cfg->ctrl, channel_id, adap_layer);
/* Do RCU sync before initiating cleanup */
synchronize_rcu();
if (adap_layer->ctrlcmd != NULL)
adap_layer->ctrlcmd(adap_layer, CAIF_CTRLCMD_DEINIT_RSP, 0);
- return ret;
+ return 0;
}
EXPORT_SYMBOL(caif_disconnect_client);
@@ -400,6 +387,14 @@ cfcnfg_linkup_rsp(struct cflayer *layer, u8 channel_id, enum cfctrl_srv serv,
struct cfcnfg_phyinfo *phyinfo;
struct net_device *netdev;
+ if (channel_id == 0) {
+ pr_warn("received channel_id zero\n");
+ if (adapt_layer != NULL && adapt_layer->ctrlcmd != NULL)
+ adapt_layer->ctrlcmd(adapt_layer,
+ CAIF_CTRLCMD_INIT_FAIL_RSP, 0);
+ return;
+ }
+
rcu_read_lock();
if (adapt_layer == NULL) {
@@ -523,7 +518,6 @@ got_phyid:
phyinfo->use_stx = stx;
phyinfo->use_fcs = fcs;
- phy_layer->type = phy_type;
frml = cffrml_create(phyid, fcs);
if (!frml) {
diff --git a/net/caif/cfmuxl.c b/net/caif/cfmuxl.c
index 2a56df7..3a66b8c 100644
--- a/net/caif/cfmuxl.c
+++ b/net/caif/cfmuxl.c
@@ -62,16 +62,6 @@ struct cflayer *cfmuxl_create(void)
return &this->layer;
}
-int cfmuxl_set_uplayer(struct cflayer *layr, struct cflayer *up, u8 linkid)
-{
- struct cfmuxl *muxl = container_obj(layr);
-
- spin_lock_bh(&muxl->receive_lock);
- list_add_rcu(&up->node, &muxl->srvl_list);
- spin_unlock_bh(&muxl->receive_lock);
- return 0;
-}
-
int cfmuxl_set_dnlayer(struct cflayer *layr, struct cflayer *dn, u8 phyid)
{
struct cfmuxl *muxl = (struct cfmuxl *) layr;
@@ -93,6 +83,24 @@ static struct cflayer *get_from_id(struct list_head *list, u16 id)
return NULL;
}
+int cfmuxl_set_uplayer(struct cflayer *layr, struct cflayer *up, u8 linkid)
+{
+ struct cfmuxl *muxl = container_obj(layr);
+ struct cflayer *old;
+
+ spin_lock_bh(&muxl->receive_lock);
+
+ /* Two entries with same id is wrong, so remove old layer from mux */
+ old = get_from_id(&muxl->srvl_list, linkid);
+ if (old != NULL)
+ list_del_rcu(&old->node);
+
+ list_add_rcu(&up->node, &muxl->srvl_list);
+ spin_unlock_bh(&muxl->receive_lock);
+
+ return 0;
+}
+
struct cflayer *cfmuxl_remove_dnlayer(struct cflayer *layr, u8 phyid)
{
struct cfmuxl *muxl = container_obj(layr);
@@ -146,6 +154,11 @@ struct cflayer *cfmuxl_remove_uplayer(struct cflayer *layr, u8 id)
struct cfmuxl *muxl = container_obj(layr);
int idx = id % UP_CACHE_SIZE;
+ if (id == 0) {
+ pr_warn("Trying to remove control layer\n");
+ return NULL;
+ }
+
spin_lock_bh(&muxl->receive_lock);
up = get_from_id(&muxl->srvl_list, id);
if (up == NULL)
@@ -235,12 +248,26 @@ static void cfmuxl_ctrlcmd(struct cflayer *layr, enum caif_ctrlcmd ctrl,
{
struct cfmuxl *muxl = container_obj(layr);
struct cflayer *layer;
+ int idx;
rcu_read_lock();
list_for_each_entry_rcu(layer, &muxl->srvl_list, node) {
- if (cfsrvl_phyid_match(layer, phyid) && layer->ctrlcmd)
+
+ if (cfsrvl_phyid_match(layer, phyid) && layer->ctrlcmd) {
+
+ if ((ctrl == _CAIF_CTRLCMD_PHYIF_FLOW_OFF_IND ||
+ ctrl == CAIF_CTRLCMD_REMOTE_SHUTDOWN_IND) &&
+ layer->id != 0) {
+
+ idx = layer->id % UP_CACHE_SIZE;
+ spin_lock_bh(&muxl->receive_lock);
+ rcu_assign_pointer(muxl->up_cache[idx], NULL);
+ list_del_rcu(&layer->node);
+ spin_unlock_bh(&muxl->receive_lock);
+ }
/* NOTE: ctrlcmd is not allowed to block */
layer->ctrlcmd(layer, ctrl, phyid);
+ }
}
rcu_read_unlock();
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/5] caif: Update documentation of CAIF transmit and receive functions.
2011-05-22 21:18 [PATCH 1/5] caif: Bugfix add check NULL pointer before calling functions Sjur Brændeland
2011-05-22 21:18 ` [PATCH 2/5] caif: Fixes freeze on Link layer removal Sjur Brændeland
2011-05-22 21:18 ` [PATCH 3/5] caif: Fix freezes when running CAIF loopback device Sjur Brændeland
@ 2011-05-22 21:18 ` Sjur Brændeland
2011-05-23 0:13 ` David Miller
2011-05-22 21:18 ` [PATCH 5/5] caif: Plug memory leak for checksum error Sjur Brændeland
2011-05-23 0:13 ` [PATCH 1/5] caif: Bugfix add check NULL pointer before calling functions David Miller
4 siblings, 1 reply; 10+ messages in thread
From: Sjur Brændeland @ 2011-05-22 21:18 UTC (permalink / raw)
To: David S. Miller, netdev; +Cc: Sjur Brændeland
Trivial patch updating documentation in header files only.
Error handling of CAIF transmit errors was changed by commit:
caif: Don't resend if dev_queue_xmit fails.
This patch updates the documentation accordingly.
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
Hi Dave.
[David Miller]
>net/caif/cfctrl.c:cfctrl_enum_req() does not check the return value of
>the ->transmit() method.
>
>The documentation for this method states that one error, ownership
>of the packet is transferred back to the caller, which means that we
>have a leak here.
>
>Please fix this bug.
In this case the documentation is buggy, Error handling was of
transmit errors was changed the April 11th by commit
"caif: Don't resend if dev_queue_xmit fails", but documentation was not
updated.
I'm sorry about this slip-up.
Regards,
Sjur
include/net/caif/caif_layer.h | 36 +++++++++++++++++++++---------------
1 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/include/net/caif/caif_layer.h b/include/net/caif/caif_layer.h
index c8b07a9..35bc788 100644
--- a/include/net/caif/caif_layer.h
+++ b/include/net/caif/caif_layer.h
@@ -15,7 +15,6 @@ struct cfpktq;
struct caif_payload_info;
struct caif_packet_funcs;
-
#define CAIF_LAYER_NAME_SZ 16
/**
@@ -33,7 +32,6 @@ do { \
} \
} while (0)
-
/**
* enum caif_ctrlcmd - CAIF Stack Control Signaling sent in layer.ctrlcmd().
*
@@ -141,7 +139,7 @@ enum caif_direction {
* - All layers must use this structure. If embedding it, then place this
* structure first in the layer specific structure.
*
- * - Each layer should not depend on any others layer private data.
+ * - Each layer should not depend on any others layer's private data.
*
* - In order to send data upwards do
* layer->up->receive(layer->up, packet);
@@ -155,16 +153,23 @@ struct cflayer {
struct list_head node;
/*
- * receive() - Receive Function.
+ * receive() - Receive Function (non-blocking).
* Contract: Each layer must implement a receive function passing the
* CAIF packets upwards in the stack.
* Packet handling rules:
- * - The CAIF packet (cfpkt) cannot be accessed after
- * passing it to the next layer using up->receive().
+ * - The CAIF packet (cfpkt) ownership is passed to the
+ * called receive function. This means that the the
+ * packet cannot be accessed after passing it to the
+ * above layer using up->receive().
+ *
* - If parsing of the packet fails, the packet must be
- * destroyed and -1 returned from the function.
+ * destroyed and negative error code returned
+ * from the function.
+ * EXCEPTION: If the framing layer (cffrml) returns
+ * -EILSEQ, the packet is not freed.
+ *
* - If parsing succeeds (and above layers return OK) then
- * the function must return a value > 0.
+ * the function must return a value >= 0.
*
* Returns result < 0 indicates an error, 0 or positive value
* indicates success.
@@ -176,7 +181,7 @@ struct cflayer {
int (*receive)(struct cflayer *layr, struct cfpkt *cfpkt);
/*
- * transmit() - Transmit Function.
+ * transmit() - Transmit Function (non-blocking).
* Contract: Each layer must implement a transmit function passing the
* CAIF packet downwards in the stack.
* Packet handling rules:
@@ -185,15 +190,16 @@ struct cflayer {
* cannot be accessed after passing it to the below
* layer using dn->transmit().
*
- * - If transmit fails, however, the ownership is returned
- * to thecaller. The caller of "dn->transmit()" must
- * destroy or resend packet.
+ * - Upon error the packet ownership is still passed on,
+ * so the packet shall be freed where error is detected.
+ * Callers of the transmit function shall not free packets,
+ * but errors shall be returned.
*
* - Return value less than zero means error, zero or
* greater than zero means OK.
*
- * result < 0 indicates an error, 0 or positive value
- * indicate success.
+ * Returns result < 0 indicates an error, 0 or positive value
+ * indicates success.
*
* @layr: Pointer to the current layer the receive function
* isimplemented for (this pointer).
@@ -202,7 +208,7 @@ struct cflayer {
int (*transmit) (struct cflayer *layr, struct cfpkt *cfpkt);
/*
- * cttrlcmd() - Control Function upwards in CAIF Stack.
+ * cttrlcmd() - Control Function upwards in CAIF Stack (non-blocking).
* Used for signaling responses (CAIF_CTRLCMD_*_RSP)
* and asynchronous events from the modem (CAIF_CTRLCMD_*_IND)
*
--
1.7.0.4
^ permalink raw reply related [flat|nested] 10+ messages in thread