linux-kernel - Re: [PATCH] rapidio: avoid GFP_KERNEL in atomic context in riocm_send

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20160910214438.de89220445c47eb0d69c9f77@linux-foundation.org>
Date:   Sat, 10 Sep 2016 21:44:38 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Alexey Khoroshilov <khoroshilov@...ras.ru>,
        Alexandre Bounine <alexandre.bounine@....com>,
        Matt Porter <mporter@...nel.crashing.org>,
        linux-kernel@...r.kernel.org, ldv-project@...uxtesting.org
Subject: Re: [PATCH] rapidio: avoid GFP_KERNEL in atomic context in
 riocm_send_close()

On Fri, 9 Sep 2016 21:22:14 -0700 Andrew Morton <akpm@...ux-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@...ux-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@...ras.ru>
Cc: Alexandre Bounine <alexandre.bounine@....com>
Signed-off-by: Andrew Morton <akpm@...ux-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);
_