[PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close()

[PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close()

[Alexey Khoroshilov]

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

Re: [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close()

[Andrew Morton]

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);
_

Re: [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close()

[Andrew Morton]

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);
_

RE: [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send_close()

[Bounine, Alexandre]

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);
> _
> 
>