From d11a6e4495ee1fbb38b59bc88d49d050d3736929 Mon Sep 17 00:00:00 2001 From: "Prasanna S. Panchamukhi" Date: Tue, 13 Apr 2010 16:35:58 -0700 Subject: wimax i2400m: fix race condition while accessing rx_roq by using kref count This patch fixes the race condition when one thread tries to destroy the memory allocated for rx_roq, while another thread still happen to access rx_roq. Such a race condition occurs when i2400m-sdio kernel module gets unloaded, destroying the memory allocated for rx_roq while rx_roq is accessed by i2400m_rx_edata(), as explained below: $thread1 $thread2 $ void i2400m_rx_edata() $ $Access rx_roq[] $ $roq = &i2400m->rx_roq[ro_cin] $ $ i2400m_roq_[reset/queue/update_ws] $ $ $ void i2400m_rx_release(); $ $kfree(rx->roq); $ $rx->roq = NULL; $Oops! rx_roq is NULL This patch fixes the race condition using refcount approach. Signed-off-by: Prasanna S. Panchamukhi --- drivers/net/wimax/i2400m/rx.c | 44 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-) (limited to 'drivers/net/wimax/i2400m/rx.c') diff --git a/drivers/net/wimax/i2400m/rx.c b/drivers/net/wimax/i2400m/rx.c index fa2e11e5b4b9..71b697f3a68d 100644 --- a/drivers/net/wimax/i2400m/rx.c +++ b/drivers/net/wimax/i2400m/rx.c @@ -916,6 +916,25 @@ void i2400m_roq_queue_update_ws(struct i2400m *i2400m, struct i2400m_roq *roq, } +/* + * This routine destroys the memory allocated for rx_roq, when no + * other thread is accessing it. Access to rx_roq is refcounted by + * rx_roq_refcount, hence memory allocated must be destroyed when + * rx_roq_refcount becomes zero. This routine gets executed when + * rx_roq_refcount becomes zero. + */ +void i2400m_rx_roq_destroy(struct kref *ref) +{ + unsigned itr; + struct i2400m *i2400m + = container_of(ref, struct i2400m, rx_roq_refcount); + for (itr = 0; itr < I2400M_RO_CIN + 1; itr++) + __skb_queue_purge(&i2400m->rx_roq[itr].queue); + kfree(i2400m->rx_roq[0].log); + kfree(i2400m->rx_roq); + i2400m->rx_roq = NULL; +} + /* * Receive and send up an extended data packet * @@ -969,6 +988,7 @@ void i2400m_rx_edata(struct i2400m *i2400m, struct sk_buff *skb_rx, unsigned ro_needed, ro_type, ro_cin, ro_sn; struct i2400m_roq *roq; struct i2400m_roq_data *roq_data; + unsigned long flags; BUILD_BUG_ON(ETH_HLEN > sizeof(*hdr)); @@ -1007,7 +1027,16 @@ void i2400m_rx_edata(struct i2400m *i2400m, struct sk_buff *skb_rx, ro_cin = (reorder >> I2400M_RO_CIN_SHIFT) & I2400M_RO_CIN; ro_sn = (reorder >> I2400M_RO_SN_SHIFT) & I2400M_RO_SN; + spin_lock_irqsave(&i2400m->rx_lock, flags); roq = &i2400m->rx_roq[ro_cin]; + if (roq == NULL) { + kfree_skb(skb); /* rx_roq is already destroyed */ + spin_unlock_irqrestore(&i2400m->rx_lock, flags); + goto error; + } + kref_get(&i2400m->rx_roq_refcount); + spin_unlock_irqrestore(&i2400m->rx_lock, flags); + roq_data = (struct i2400m_roq_data *) &skb->cb; roq_data->sn = ro_sn; roq_data->cs = cs; @@ -1034,6 +1063,10 @@ void i2400m_rx_edata(struct i2400m *i2400m, struct sk_buff *skb_rx, default: dev_err(dev, "HW BUG? unknown reorder type %u\n", ro_type); } + + spin_lock_irqsave(&i2400m->rx_lock, flags); + kref_put(&i2400m->rx_roq_refcount, i2400m_rx_roq_destroy); + spin_unlock_irqrestore(&i2400m->rx_lock, flags); } else i2400m_net_erx(i2400m, skb, cs); @@ -1344,6 +1377,7 @@ int i2400m_rx_setup(struct i2400m *i2400m) __i2400m_roq_init(&i2400m->rx_roq[itr]); i2400m->rx_roq[itr].log = &rd[itr]; } + kref_init(&i2400m->rx_roq_refcount); } return 0; @@ -1357,12 +1391,12 @@ error_roq_alloc: /* Tear down the RX queue and infrastructure */ void i2400m_rx_release(struct i2400m *i2400m) { + unsigned long flags; + if (i2400m->rx_reorder) { - unsigned itr; - for(itr = 0; itr < I2400M_RO_CIN + 1; itr++) - __skb_queue_purge(&i2400m->rx_roq[itr].queue); - kfree(i2400m->rx_roq[0].log); - kfree(i2400m->rx_roq); + spin_lock_irqsave(&i2400m->rx_lock, flags); + kref_put(&i2400m->rx_roq_refcount, i2400m_rx_roq_destroy); + spin_unlock_irqrestore(&i2400m->rx_lock, flags); } /* at this point, nothing can be received... */ i2400m_report_hook_flush(i2400m); -- cgit v1.2.3 From 0809a7bbe8fbcb4e899b0a3224d8461bd74987e0 Mon Sep 17 00:00:00 2001 From: "Prasanna S. Panchamukhi" Date: Tue, 13 Apr 2010 16:36:10 -0700 Subject: wimax/i2400m: fix incorrect handling of type 2 and 3 RX messages According to Intel Wimax i3200, i5x50 and i6x60 device specification documents, the host driver must not reset the device if the normalized sequence numbers are greater than 1023 for type 2 and type 3 RX messages. This patch removes the code that incorrectly used to reset the device. Signed-off-by: Prasanna S. Panchamukhi --- drivers/net/wimax/i2400m/rx.c | 51 ++++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 25 deletions(-) (limited to 'drivers/net/wimax/i2400m/rx.c') diff --git a/drivers/net/wimax/i2400m/rx.c b/drivers/net/wimax/i2400m/rx.c index 71b697f3a68d..66f968a24d49 100644 --- a/drivers/net/wimax/i2400m/rx.c +++ b/drivers/net/wimax/i2400m/rx.c @@ -743,12 +743,12 @@ unsigned __i2400m_roq_update_ws(struct i2400m *i2400m, struct i2400m_roq *roq, unsigned new_nws, nsn_itr; new_nws = __i2400m_roq_nsn(roq, sn); - if (unlikely(new_nws >= 1024) && d_test(1)) { - dev_err(dev, "SW BUG? __update_ws new_nws %u (sn %u ws %u)\n", - new_nws, sn, roq->ws); - WARN_ON(1); - i2400m_roq_log_dump(i2400m, roq); - } + /* + * For type 2(update_window_start) rx messages, there is no + * need to check if the normalized sequence number is greater 1023. + * Simply insert and deliver all packets to the host up to the + * window start. + */ skb_queue_walk_safe(&roq->queue, skb_itr, tmp_itr) { roq_data_itr = (struct i2400m_roq_data *) &skb_itr->cb; nsn_itr = __i2400m_roq_nsn(roq, roq_data_itr->sn); @@ -890,26 +890,27 @@ void i2400m_roq_queue_update_ws(struct i2400m *i2400m, struct i2400m_roq *roq, i2400m, roq, skb, sn); len = skb_queue_len(&roq->queue); nsn = __i2400m_roq_nsn(roq, sn); + /* + * For type 3(queue_update_window_start) rx messages, there is no + * need to check if the normalized sequence number is greater 1023. + * Simply insert and deliver all packets to the host up to the + * window start. + */ old_ws = roq->ws; - if (unlikely(nsn >= 1024)) { - dev_err(dev, "SW BUG? queue_update_ws nsn %u (sn %u ws %u)\n", - nsn, sn, roq->ws); - i2400m_roq_log_dump(i2400m, roq); - i2400m_reset(i2400m, I2400M_RT_WARM); - } else { - /* if the queue is empty, don't bother as we'd queue - * it and inmediately unqueue it -- just deliver it */ - if (len == 0) { - struct i2400m_roq_data *roq_data; - roq_data = (struct i2400m_roq_data *) &skb->cb; - i2400m_net_erx(i2400m, skb, roq_data->cs); - } - else - __i2400m_roq_queue(i2400m, roq, skb, sn, nsn); - __i2400m_roq_update_ws(i2400m, roq, sn + 1); - i2400m_roq_log_add(i2400m, roq, I2400M_RO_TYPE_PACKET_WS, - old_ws, len, sn, nsn, roq->ws); - } + /* If the queue is empty, don't bother as we'd queue + * it and immediately unqueue it -- just deliver it. + */ + if (len == 0) { + struct i2400m_roq_data *roq_data; + roq_data = (struct i2400m_roq_data *) &skb->cb; + i2400m_net_erx(i2400m, skb, roq_data->cs); + } else + __i2400m_roq_queue(i2400m, roq, skb, sn, nsn); + + __i2400m_roq_update_ws(i2400m, roq, sn + 1); + i2400m_roq_log_add(i2400m, roq, I2400M_RO_TYPE_PACKET_WS, + old_ws, len, sn, nsn, roq->ws); + d_fnend(2, dev, "(i2400m %p roq %p skb %p sn %u) = void\n", i2400m, roq, skb, sn); return; -- cgit v1.2.3 From 3e02a06ae3dce2eb804bb4afadb7067c80d6c096 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Thu, 22 Apr 2010 11:46:32 +0200 Subject: wimax: wimax_msg_alloc() returns ERR_PTR not null wimax_msg_alloc() returns an ERR_PTR and not null. I changed it to test for ERR_PTR instead of null. I also added a check in front of the kfree() because kfree() can handle null but not ERR_PTR. Signed-off-by: Dan Carpenter --- drivers/net/wimax/i2400m/rx.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'drivers/net/wimax/i2400m/rx.c') diff --git a/drivers/net/wimax/i2400m/rx.c b/drivers/net/wimax/i2400m/rx.c index 66f968a24d49..0004c686ac68 100644 --- a/drivers/net/wimax/i2400m/rx.c +++ b/drivers/net/wimax/i2400m/rx.c @@ -300,17 +300,16 @@ void i2400m_rx_ctl_ack(struct i2400m *i2400m, d_printf(1, dev, "Huh? waiter for command reply cancelled\n"); goto error_waiter_cancelled; } - if (ack_skb == NULL) { + if (IS_ERR(ack_skb)) dev_err(dev, "CMD/GET/SET ack: cannot allocate SKB\n"); - i2400m->ack_skb = ERR_PTR(-ENOMEM); - } else - i2400m->ack_skb = ack_skb; + i2400m->ack_skb = ack_skb; spin_unlock_irqrestore(&i2400m->rx_lock, flags); complete(&i2400m->msg_completion); return; error_waiter_cancelled: - kfree_skb(ack_skb); + if (!IS_ERR(ack_skb)) + kfree_skb(ack_skb); error_no_waiter: spin_unlock_irqrestore(&i2400m->rx_lock, flags); return; -- cgit v1.2.3 From 9d7fdf1ba9d5b8963bf8ffe29eea17f508e81bde Mon Sep 17 00:00:00 2001 From: Prasanna S Panchamukhi Date: Tue, 17 Nov 2009 18:29:35 -0800 Subject: wimax/i2400m: Move module params to other file so they can be static This patch moves the module parameters to the file where they can be avoided to be global and allow them to be static. The module param : idle_mode_disabled and power_save_disabled are moved from driver.c to control.c. Also these module parameters are declared to be static as they are not required to be global anymore. The module param : rx_reorder_disabled is moved from driver.c file to rx.c file. Also this parameter is declated as static as it is not required to be global anymore. Signed-off-by: Prasanna S Panchamukhi --- drivers/net/wimax/i2400m/control.c | 15 +++++++++++++++ drivers/net/wimax/i2400m/driver.c | 19 ------------------- drivers/net/wimax/i2400m/i2400m.h | 6 ------ drivers/net/wimax/i2400m/rx.c | 5 +++++ 4 files changed, 20 insertions(+), 25 deletions(-) (limited to 'drivers/net/wimax/i2400m/rx.c') diff --git a/drivers/net/wimax/i2400m/control.c b/drivers/net/wimax/i2400m/control.c index 6180772dcc09..0c1aa8867531 100644 --- a/drivers/net/wimax/i2400m/control.c +++ b/drivers/net/wimax/i2400m/control.c @@ -83,6 +83,21 @@ #define D_SUBMODULE control #include "debug-levels.h" +static int i2400m_idle_mode_disabled;/* 0 (idle mode enabled) by default */ +module_param_named(idle_mode_disabled, i2400m_idle_mode_disabled, int, 0644); +MODULE_PARM_DESC(idle_mode_disabled, + "If true, the device will not enable idle mode negotiation " + "with the base station (when connected) to save power."); + +/* 0 (power saving enabled) by default */ +static int i2400m_power_save_disabled; +module_param_named(power_save_disabled, i2400m_power_save_disabled, int, 0644); +MODULE_PARM_DESC(power_save_disabled, + "If true, the driver will not tell the device to enter " + "power saving mode when it reports it is ready for it. " + "False by default (so the device is told to do power " + "saving)."); + int i2400m_passive_mode; /* 0 (passive mode disabled) by default */ module_param_named(passive_mode, i2400m_passive_mode, int, 0644); MODULE_PARM_DESC(passive_mode, diff --git a/drivers/net/wimax/i2400m/driver.c b/drivers/net/wimax/i2400m/driver.c index 39cf96a90d7a..66bdb5d6cd34 100644 --- a/drivers/net/wimax/i2400m/driver.c +++ b/drivers/net/wimax/i2400m/driver.c @@ -75,25 +75,6 @@ #include "debug-levels.h" -int i2400m_idle_mode_disabled; /* 0 (idle mode enabled) by default */ -module_param_named(idle_mode_disabled, i2400m_idle_mode_disabled, int, 0644); -MODULE_PARM_DESC(idle_mode_disabled, - "If true, the device will not enable idle mode negotiation " - "with the base station (when connected) to save power."); - -int i2400m_rx_reorder_disabled; /* 0 (rx reorder enabled) by default */ -module_param_named(rx_reorder_disabled, i2400m_rx_reorder_disabled, int, 0644); -MODULE_PARM_DESC(rx_reorder_disabled, - "If true, RX reordering will be disabled."); - -int i2400m_power_save_disabled; /* 0 (power saving enabled) by default */ -module_param_named(power_save_disabled, i2400m_power_save_disabled, int, 0644); -MODULE_PARM_DESC(power_save_disabled, - "If true, the driver will not tell the device to enter " - "power saving mode when it reports it is ready for it. " - "False by default (so the device is told to do power " - "saving)."); - static char i2400m_debug_params[128]; module_param_string(debug, i2400m_debug_params, sizeof(i2400m_debug_params), 0644); diff --git a/drivers/net/wimax/i2400m/i2400m.h b/drivers/net/wimax/i2400m/i2400m.h index 1babc55b1312..fa74777fd65f 100644 --- a/drivers/net/wimax/i2400m/i2400m.h +++ b/drivers/net/wimax/i2400m/i2400m.h @@ -885,7 +885,6 @@ extern int i2400m_rx(struct i2400m *, struct sk_buff *); extern struct i2400m_msg_hdr *i2400m_tx_msg_get(struct i2400m *, size_t *); extern void i2400m_tx_msg_sent(struct i2400m *); -extern int i2400m_power_save_disabled; /* * Utility functions @@ -992,10 +991,5 @@ extern int i2400m_barker_db_init(const char *); extern void i2400m_barker_db_exit(void); -/* Module parameters */ - -extern int i2400m_idle_mode_disabled; -extern int i2400m_rx_reorder_disabled; - #endif /* #ifndef __I2400M_H__ */ diff --git a/drivers/net/wimax/i2400m/rx.c b/drivers/net/wimax/i2400m/rx.c index 0004c686ac68..c835ae8b89ce 100644 --- a/drivers/net/wimax/i2400m/rx.c +++ b/drivers/net/wimax/i2400m/rx.c @@ -155,6 +155,11 @@ #define D_SUBMODULE rx #include "debug-levels.h" +static int i2400m_rx_reorder_disabled; /* 0 (rx reorder enabled) by default */ +module_param_named(rx_reorder_disabled, i2400m_rx_reorder_disabled, int, 0644); +MODULE_PARM_DESC(rx_reorder_disabled, + "If true, RX reordering will be disabled."); + struct i2400m_report_hook_args { struct sk_buff *skb_rx; const struct i2400m_l3l4_hdr *l3l4_hdr; -- cgit v1.2.3