On Fri, 9 Sep 2016 23:43:35 +0300 Alexey Khoroshilov <khoroshi...@ispras.ru>
wrote:
> riocm_send_close() is called from rio_cm_shutdown() and riocm_ch_close().
> The first site is within section protected by idr_lock spinlock,
> while the second one is not in atomic context.
>
> The patch adds gfp_t argument to allocate memory appropriately to
> the corresponding context.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> --- a/drivers/rapidio/rio_cm.c
> +++ b/drivers/rapidio/rio_cm.c
> @@ -1395,7 +1395,7 @@ static void riocm_ch_free(struct kref *ref)
> complete(&ch->comp_close);
> }
>
> -static int riocm_send_close(struct rio_channel *ch)
> +static int riocm_send_close(struct rio_channel *ch, gfp_t gfp)
> {
> struct rio_ch_chan_hdr *hdr;
> int ret;
> @@ -1404,7 +1404,7 @@ static int riocm_send_close(struct rio_channel *ch)
> * Send CH_CLOSE notification to the remote RapidIO device
> */
>
> - hdr = kzalloc(sizeof(*hdr), GFP_KERNEL);
> + hdr = kzalloc(sizeof(*hdr), gfp);
> if (hdr == NULL)
> return -ENOMEM;
>
> @@ -1450,7 +1450,7 @@ static int riocm_ch_close(struct rio_channel *ch)
>
> state = riocm_exch(ch, RIO_CM_DESTROYING);
> if (state == RIO_CM_CONNECTED)
> - riocm_send_close(ch);
> + riocm_send_close(ch, GFP_KERNEL);
>
> complete_all(&ch->comp);
>
> @@ -2254,7 +2254,7 @@ static int rio_cm_shutdown(struct notifier_block *nb,
> unsigned long code,
> idr_for_each_entry(&ch_idr, ch, i) {
> riocm_debug(EXIT, "close ch %d", ch->id);
> if (ch->state == RIO_CM_CONNECTED)
> - riocm_send_close(ch);
> + riocm_send_close(ch, GFP_ATOMIC);
Switching to GFP_ATOMIC is undesirable - GFP_ATOMIC is less reliable
and can drain memory reserves which are required by code which *must*
use GFP_ATOMIC, such as interrupt handlers.
Why not just make `hdr' a local? It isn't very large and the code
becomes smaller and faster.
From: Andrew Morton <a...@linux-foundation.org>
Subject: drivers/rapidio/rio_cm.c: avoid GFP_KERNEL in atomic context
riocm_send_close() is called from rio_cm_shutdown() under
spin_lock_bh(idr_lock), but riocm_send_close() uses a GFP_KERNEL
allocation.
Fix this by making `hdr' a local variable rather than obtaining it
via kmalloc().
Found by Linux Driver Verification project (linuxtesting.org).
Link:
http://lkml.kernel.org/r/1473453815-15914-1-git-send-email-khoroshi...@ispras.ru
Reported-by: Alexey Khoroshilov <khoroshi...@ispras.ru>
Cc: Alexandre Bounine <alexandre.boun...@idt.com>
Signed-off-by: Andrew Morton <a...@linux-foundation.org>
---
drivers/rapidio/rio_cm.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff -puN
drivers/rapidio/rio_cm.c~drivers-rapidio-rio_cmc-avoid-gfp_kernel-in-atomic-context
drivers/rapidio/rio_cm.c
---
a/drivers/rapidio/rio_cm.c~drivers-rapidio-rio_cmc-avoid-gfp_kernel-in-atomic-context
+++ a/drivers/rapidio/rio_cm.c
@@ -1397,36 +1397,32 @@ static void riocm_ch_free(struct kref *r
static int riocm_send_close(struct rio_channel *ch)
{
- struct rio_ch_chan_hdr *hdr;
+ struct rio_ch_chan_hdr hdr;
int ret;
/*
* Send CH_CLOSE notification to the remote RapidIO device
*/
- hdr = kzalloc(sizeof(*hdr), GFP_KERNEL);
- if (hdr == NULL)
- return -ENOMEM;
-
- hdr->bhdr.src_id = htonl(ch->loc_destid);
- hdr->bhdr.dst_id = htonl(ch->rem_destid);
- hdr->bhdr.src_mbox = cmbox;
- hdr->bhdr.dst_mbox = cmbox;
- hdr->bhdr.type = RIO_CM_CHAN;
- hdr->ch_op = CM_CONN_CLOSE;
- hdr->dst_ch = htons(ch->rem_channel);
- hdr->src_ch = htons(ch->id);
+ memset(&hdr, 0, sizeof(hdr));
+ hdr.bhdr.src_id = htonl(ch->loc_destid);
+ hdr.bhdr.dst_id = htonl(ch->rem_destid);
+ hdr.bhdr.src_mbox = cmbox;
+ hdr.bhdr.dst_mbox = cmbox;
+ hdr.bhdr.type = RIO_CM_CHAN;
+ hdr.ch_op = CM_CONN_CLOSE;
+ hdr.dst_ch = htons(ch->rem_channel);
+ hdr.src_ch = htons(ch->id);
/* ATTN: the function call below relies on the fact that underlying
* add_outb_message() routine copies TX data into its internal transfer
* buffer. Needs to be reviewed if switched to direct buffer mode.
*/
- ret = riocm_post_send(ch->cmdev, ch->rdev, hdr, sizeof(*hdr));
+ ret = riocm_post_send(ch->cmdev, ch->rdev, &hdr, sizeof(hdr));
if (ret == -EBUSY && !riocm_queue_req(ch->cmdev, ch->rdev,
- hdr, sizeof(*hdr)))
+ &hdr, sizeof(hdr)))
return 0;
- kfree(hdr);
if (ret)
riocm_error("ch(%d) send CLOSE failed (ret=%d)", ch->id, ret);
_
