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

From: Bounine, Alexandre
Date: Wed Sep 14 2016 - 15:49:22 EST


On Sunday, Sep 11, 2016 12:45 AM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx: 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@xxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxx
> Reported-by: Alexey Khoroshilov <khoroshilov@xxxxxxxxx>
> Cc: Alexandre Bounine <alexandre.bounine@xxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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);
> _
>
>