* [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close()
@ 2016-09-09 20:43 Alexey Khoroshilov
2016-09-10 4:22 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Khoroshilov @ 2016-09-09 20:43 UTC (permalink / raw)
To: Alexandre Bounine
Cc: Alexey Khoroshilov, Matt Porter, linux-kernel, ldv-project
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).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/rapidio/rio_cm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/rapidio/rio_cm.c b/drivers/rapidio/rio_cm.c
index 3fa17ac8df54..ff5ed8970309 100644
--- 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);
}
spin_unlock_bh(&idr_lock);
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close()
2016-09-09 20:43 [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close() Alexey Khoroshilov
@ 2016-09-10 4:22 ` Andrew Morton
2016-09-11 4:44 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2016-09-10 4:22 UTC (permalink / raw)
To: Alexey Khoroshilov
Cc: Alexandre Bounine, Matt Porter, linux-kernel, ldv-project
On Fri, 9 Sep 2016 23:43:35 +0300 Alexey Khoroshilov <khoroshilov@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 <akpm@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-khoroshilov@ispras.ru
Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Cc: Alexandre Bounine <alexandre.bounine@idt.com>
Signed-off-by: Andrew Morton <akpm@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);
_
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close()
2016-09-10 4:22 ` Andrew Morton
@ 2016-09-11 4:44 ` Andrew Morton
2016-09-14 19:48 ` Bounine, Alexandre
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2016-09-11 4:44 UTC (permalink / raw)
To: Alexey Khoroshilov, Alexandre Bounine, Matt Porter, linux-kernel,
ldv-project
On Fri, 9 Sep 2016 21:22:14 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> Why not just make `hdr' a local? It isn't very large and the code
> becomes smaller and faster.
Actually, doing it this way saves some code size and is faster:
From: Andrew Morton <akpm@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 local `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-khoroshilov@ispras.ru
Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Cc: Alexandre Bounine <alexandre.bounine@idt.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/rapidio/rio_cm.c | 38 ++++++++++++++++---------------------
1 file changed, 17 insertions(+), 21 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
@@ -1395,38 +1395,34 @@ static void riocm_ch_free(struct kref *r
complete(&ch->comp_close);
}
+/*
+ * Send CH_CLOSE notification to the remote RapidIO device
+ */
static int riocm_send_close(struct rio_channel *ch)
{
- 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);
+ struct rio_ch_chan_hdr hdr = {
+ .bhdr = {
+ .src_id = htonl(ch->loc_destid),
+ .dst_id = htonl(ch->rem_destid),
+ .src_mbox = cmbox,
+ .dst_mbox = cmbox,
+ .type = RIO_CM_CHAN,
+ },
+ .ch_op = CM_CONN_CLOSE,
+ .dst_ch = htons(ch->rem_channel),
+ .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);
_
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close()
2016-09-11 4:44 ` Andrew Morton
@ 2016-09-14 19:48 ` Bounine, Alexandre
0 siblings, 0 replies; 4+ messages in thread
From: Bounine, Alexandre @ 2016-09-14 19:48 UTC (permalink / raw)
To: Andrew Morton, Alexey Khoroshilov, Matt Porter, linux-kernel,
ldv-project
On Sunday, Sep 11, 2016 12:45 AM Andrew Morton <akpm@linux-foundation.org: wrote:
>
> On Fri, 9 Sep 2016 21:22:14 -0700 Andrew Morton <akpm@linux-
> foundation.org> wrote:
>
> > Why not just make `hdr' a local? It isn't very large and the code
> > becomes smaller and faster.
>
> Actually, doing it this way saves some code size and is faster:
>
Using local 'hdr' will cause a reference to an illegal memory in case when
"close" request is placed into priority queue due to lack of free buffer
descriptors in low-level HW driver. This can happen during high intensity
messaging traffic - when riocm_send_close() is called from riocm_ch_close().
I will rework rio_cm_shutdown() to take riocm_send_close() call out of spin-lock
protected code area.
>
> From: Andrew Morton <akpm@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 local `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-
> khoroshilov@ispras.ru
> Reported-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> Cc: Alexandre Bounine <alexandre.bounine@idt.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> drivers/rapidio/rio_cm.c | 38 ++++++++++++++++---------------------
> 1 file changed, 17 insertions(+), 21 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
> @@ -1395,38 +1395,34 @@ static void riocm_ch_free(struct kref *r
> complete(&ch->comp_close);
> }
>
> +/*
> + * Send CH_CLOSE notification to the remote RapidIO device
> + */
> static int riocm_send_close(struct rio_channel *ch)
> {
> - 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);
> + struct rio_ch_chan_hdr hdr = {
> + .bhdr = {
> + .src_id = htonl(ch->loc_destid),
> + .dst_id = htonl(ch->rem_destid),
> + .src_mbox = cmbox,
> + .dst_mbox = cmbox,
> + .type = RIO_CM_CHAN,
> + },
> + .ch_op = CM_CONN_CLOSE,
> + .dst_ch = htons(ch->rem_channel),
> + .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);
> _
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-14 19:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-09 20:43 [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close() Alexey Khoroshilov
2016-09-10 4:22 ` Andrew Morton
2016-09-11 4:44 ` Andrew Morton
2016-09-14 19:48 ` Bounine, Alexandre
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).