From: Divy Le Ray <divy@chelsio.com>
To: Neil Horman <nhorman@tuxdriver.com>
Cc: netdev@vger.kernel.org, Steve Wise <swise@chelsio.com>,
"David S. Miller" <davem@davemloft.net>,
Karen Xie <kxie@chelsio.com>
Subject: Re: [PATCH] cxgb3i: ref count cdev access to prevent modification while in use
Date: Mon, 25 Jul 2011 14:19:01 -0700 [thread overview]
Message-ID: <4E2DDDC5.8040405@chelsio.com> (raw)
In-Reply-To: <1311623817-6417-1-git-send-email-nhorman@tuxdriver.com>
On 07/25/2011 12:56 PM, Neil Horman wrote:
>
> This oops was reported recently:
> d:mon> e
> cpu 0xd: Vector: 300 (Data Access) at [c0000000fd4c7120]
> pc: d00000000076f194: .t3_l2t_get+0x44/0x524 [cxgb3]
> lr: d000000000b02108: .init_act_open+0x150/0x3d4 [cxgb3i]
> sp: c0000000fd4c73a0
> msr: 8000000000009032
> dar: 0
> dsisr: 40000000
> current = 0xc0000000fd640d40
> paca = 0xc00000000054ff80
> pid = 5085, comm = iscsid
> d:mon> t
> [c0000000fd4c7450] d000000000b02108 .init_act_open+0x150/0x3d4 [cxgb3i]
> [c0000000fd4c7500] d000000000e45378 .cxgbi_ep_connect+0x784/0x8e8
> [libcxgbi]
> [c0000000fd4c7650] d000000000db33f0 .iscsi_if_rx+0x71c/0xb18
> [scsi_transport_iscsi2]
> [c0000000fd4c7740] c000000000370c9c .netlink_data_ready+0x40/0xa4
> [c0000000fd4c77c0] c00000000036f010 .netlink_sendskb+0x4c/0x9c
> [c0000000fd4c7850] c000000000370c18 .netlink_sendmsg+0x358/0x39c
> [c0000000fd4c7950] c00000000033be24 .sock_sendmsg+0x114/0x1b8
> [c0000000fd4c7b50] c00000000033d208 .sys_sendmsg+0x218/0x2ac
> [c0000000fd4c7d70] c00000000033f55c .sys_socketcall+0x228/0x27c
> [c0000000fd4c7e30] c0000000000086a4 syscall_exit+0x0/0x40
> --- Exception: c01 (System Call) at 00000080da560cfc
>
> The root cause was an EEH error, which sent us down the offload_close
> path in
> the cxgb3 driver, which in turn sets cdev->lldev to NULL, without
> regard for
> upper layer driver (like the cxgbi drivers) which might have execution
> contexts
> in the middle of its use. The result is the oops above, when
> t3_l2t_get attempts
> to dereference cdev->lldev right after the EEH error handler sets it
> to NULL.
>
> The fix is to reference count the cdev structure. When an EEH error
> occurs, the
> shutdown path:
> t3_adapter_error->offload_close->cxgb3i_remove_clients->cxgb3i_dev_close
> will now block until such time as the cdev pointer has a use count of
> zero.
> This coupled with the fact that lookups will now skip finding any
> registered
> cdev's in cxgbi_device_find_by_[lldev|netdev] with the
> CXGBI_FLAG_ADAPTER_RESET
> bit set ensures that on an EEH, the setting of lldev to NULL in
> offload_close
> will only happen after there are no longer any active users of the data
> structure.
>
> This has been tested by the reporter and shown to fix the reproted oops
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Divy Le Ray <divy@chelsio.com>
> CC: Steve Wise <swise@chelsio.com>
> CC: "David S. Miller" <davem@davemloft.net>
>
Also cc-ing Karen.
> ---
> drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 8 +++++++-
> drivers/scsi/cxgbi/libcxgbi.c | 9 +++++++++
> drivers/scsi/cxgbi/libcxgbi.h | 3 +++
> 3 files changed, 19 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> index abc7b12..7d752cd 100644
> --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> @@ -1301,9 +1301,13 @@ static void cxgb3i_dev_close(struct t3cdev *t3dev)
>
> if (!cdev || cdev->flags & CXGBI_FLAG_ADAPTER_RESET) {
> pr_info("0x%p close, f 0x%x.\n", cdev, cdev ?
> cdev->flags : 0);
> + if (cdev)
> + cdev_put(cdev);
> + while (cdev && atomic_read(&cdev->use_count) != 0)
> + msleep(1);
> return;
> }
> -
> + cdev_put(cdev);
> cxgbi_device_unregister(cdev);
> }
>
> @@ -1318,6 +1322,7 @@ static void cxgb3i_dev_open(struct t3cdev *t3dev)
> int i, err;
>
> if (cdev) {
> + cdev_put(cdev);
> pr_info("0x%p, updating.\n", cdev);
> return;
> }
> @@ -1390,6 +1395,7 @@ static void cxgb3i_dev_event_handler(struct
> t3cdev *t3dev, u32 event, u32 port)
> cdev->flags &= ~CXGBI_FLAG_ADAPTER_RESET;
> break;
> }
> + cdev_put(cdev);
> }
>
> /**
> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
> index 77ac217..eb5625d 100644
> --- a/drivers/scsi/cxgbi/libcxgbi.c
> +++ b/drivers/scsi/cxgbi/libcxgbi.c
> @@ -181,6 +181,9 @@ struct cxgbi_device
> *cxgbi_device_find_by_lldev(void *lldev)
> mutex_lock(&cdev_mutex);
> list_for_each_entry_safe(cdev, tmp, &cdev_list, list_head) {
> if (cdev->lldev == lldev) {
> + if (cdev->flags & CXGBI_FLAG_ADAPTER_RESET)
> + continue;
> + cdev_hold(cdev);
> mutex_unlock(&cdev_mutex);
> return cdev;
> }
> @@ -210,7 +213,10 @@ static struct cxgbi_device
> *cxgbi_device_find_by_netdev(struct net_device *ndev,
> list_for_each_entry_safe(cdev, tmp, &cdev_list, list_head) {
> for (i = 0; i < cdev->nports; i++) {
> if (ndev == cdev->ports[i]) {
> + if (cdev->flags &
> CXGBI_FLAG_ADAPTER_RESET)
> + continue;
> cdev->hbas[i]->vdev = vdev;
> + cdev_hold(cdev);
> mutex_unlock(&cdev_mutex);
> if (port)
> *port = i;
> @@ -542,6 +548,8 @@ rel_rt:
> if (csk)
> cxgbi_sock_closed(csk);
> err_out:
> + if (cdev)
> + cdev_put(cdev);
> return ERR_PTR(err);
> }
>
> @@ -2491,6 +2499,7 @@ struct iscsi_endpoint *cxgbi_ep_connect(struct
> Scsi_Host *shost,
> return ep;
>
> release_conn:
> + cdev_put(&csk->cdev);
> cxgbi_sock_put(csk);
> cxgbi_sock_closed(csk);
> err_out:
> diff --git a/drivers/scsi/cxgbi/libcxgbi.h b/drivers/scsi/cxgbi/libcxgbi.h
> index 9267844..aad1749 100644
> --- a/drivers/scsi/cxgbi/libcxgbi.h
> +++ b/drivers/scsi/cxgbi/libcxgbi.h
> @@ -514,6 +514,7 @@ struct cxgbi_device {
> unsigned int flags;
> struct net_device **ports;
> void *lldev;
> + atomic_t use_count;
> struct cxgbi_hba **hbas;
> const unsigned short *mtus;
> unsigned char nmtus;
> @@ -557,6 +558,8 @@ struct cxgbi_device {
> void *dd_data;
> };
> #define cxgbi_cdev_priv(cdev) ((cdev)->dd_data)
> +#define cdev_hold(x) do {atomic_inc(&x->use_count);} while(0)
> +#define cdev_put(x) do {atomic_dec(&x->use_count);} while(0)
>
> struct cxgbi_conn {
> struct cxgbi_endpoint *cep;
> --
> 1.7.3.4
>
next prev parent reply other threads:[~2011-07-25 22:24 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-25 19:56 [PATCH] cxgb3i: ref count cdev access to prevent modification while in use Neil Horman
2011-07-25 21:19 ` Divy Le Ray [this message]
2011-07-26 10:50 ` Neil Horman
2011-07-26 16:42 ` Karen Xie
2011-08-01 11:18 ` Neil Horman
2011-08-01 14:42 ` Steve Wise
2011-08-02 0:36 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2011-08-02 2:00 Neil Horman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4E2DDDC5.8040405@chelsio.com \
--to=divy@chelsio.com \
--cc=davem@davemloft.net \
--cc=kxie@chelsio.com \
--cc=netdev@vger.kernel.org \
--cc=nhorman@tuxdriver.com \
--cc=swise@chelsio.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).