On Sunday, Sep 11, 2016 12:45 AM Andrew Morton <a...@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 <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 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- > 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 | 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); > _ > >
