Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Wen Yang <wen.yang99@zte.com.cn>
To:     boris.ostrovsky@oracle.com, jgross@suse.com, sstabellini@kernel.org
Cc:     xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org,
        zhong.weidong@zte.com.cn, Wen Yang <wen.yang99@zte.com.cn>,
        Julia Lawall <julia.lawall@lip6.fr>
Subject: [PATCH v4] pvcalls-front: Avoid get_free_pages(GFP_KERNEL) under spinlock
Date:   Sat, 1 Dec 2018 14:33:03 +0800
Message-Id: <20181201063303.45089-1-wen.yang99@zte.com.cn>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

The problem is that we call this with a spin lock held.
The call tree is:
pvcalls=5Ffront=5Faccept() holds bedata->socket=5Flock.
    -> create=5Factive()
        -> =5F=5Fget=5Ffree=5Fpages() uses GFP=5FKERNEL

The create=5Factive() function is only called from pvcalls=5Ffront=5Faccept=
()
with a spin=5Flock held, The allocation is not allowed to sleep and
GFP=5FKERNEL is not sufficient.

This issue was detected by using the Coccinelle software.

v2: Add a function doing the allocations which is called
    outside the lock and passing the allocated data to
    create=5Factive().

v3: Use the matching deallocators i.e., free=5Fpage()
    and free=5Fpages(), respectively.

v4: It would be better to pre-populate map (struct sock=5Fmapping),
    rather than introducing one more new struct.

Suggested-by: Juergen Gross <jgross@suse.com>
Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Suggested-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
CC: Julia Lawall <julia.lawall@lip6.fr>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Juergen Gross <jgross@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: xen-devel@lists.xenproject.org
CC: linux-kernel@vger.kernel.org
---
 drivers/xen/pvcalls-front.c | 57 ++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
index 77224d8f3e6f..555c9abdf58f 100644
--- a/drivers/xen/pvcalls-front.c
+++ b/drivers/xen/pvcalls-front.c
@@ -335,13 +335,18 @@ int pvcalls=5Ffront=5Fsocket(struct socket *sock)
 	return ret;
 }
=20
-static int create=5Factive(struct sock=5Fmapping *map, int *evtchn)
+static void free=5Factive=5Fring(struct sock=5Fmapping *map)
 {
-	void *bytes;
-	int ret =3D -ENOMEM, irq =3D -1, i;
+	if (!map)
+		return;
+	free=5Fpages((unsigned long)map->active.data.in,
+			map->active.ring->ring=5Forder);
+	free=5Fpage((unsigned long)map->active.ring);
+}
=20
-	*evtchn =3D -1;
-	init=5Fwaitqueue=5Fhead(&map->active.inflight=5Fconn=5Freq);
+static int alloc=5Factive=5Fring(struct sock=5Fmapping *map)
+{
+	void *bytes;
=20
 	map->active.ring =3D (struct pvcalls=5Fdata=5Fintf *)
 		=5F=5Fget=5Ffree=5Fpage(GFP=5FKERNEL | =5F=5FGFP=5FZERO);
@@ -352,6 +357,26 @@ static int create=5Factive(struct sock=5Fmapping *map,=
 int *evtchn)
 					PVCALLS=5FRING=5FORDER);
 	if (bytes =3D=3D NULL)
 		goto out=5Ferror;
+	map->active.data.in =3D bytes;
+	map->active.data.out =3D bytes +
+		XEN=5FFLEX=5FRING=5FSIZE(PVCALLS=5FRING=5FORDER);
+
+	return 0;
+
+out=5Ferror:
+	free=5Factive=5Fring(map);
+	return -ENOMEM;
+}
+
+static int create=5Factive(struct sock=5Fmapping *map, int *evtchn)
+{
+	void *bytes;
+	int ret =3D -ENOMEM, irq =3D -1, i;
+
+	*evtchn =3D -1;
+	init=5Fwaitqueue=5Fhead(&map->active.inflight=5Fconn=5Freq);
+
+	bytes =3D map->active.data.in;
 	for (i =3D 0; i < (1 << PVCALLS=5FRING=5FORDER); i++)
 		map->active.ring->ref[i] =3D gnttab=5Fgrant=5Fforeign=5Faccess(
 			pvcalls=5Ffront=5Fdev->otherend=5Fid,
@@ -361,10 +386,6 @@ static int create=5Factive(struct sock=5Fmapping *map,=
 int *evtchn)
 		pvcalls=5Ffront=5Fdev->otherend=5Fid,
 		pfn=5Fto=5Fgfn(virt=5Fto=5Fpfn((void *)map->active.ring)), 0);
=20
-	map->active.data.in =3D bytes;
-	map->active.data.out =3D bytes +
-		XEN=5FFLEX=5FRING=5FSIZE(PVCALLS=5FRING=5FORDER);
-
 	ret =3D xenbus=5Falloc=5Fevtchn(pvcalls=5Ffront=5Fdev, evtchn);
 	if (ret)
 		goto out=5Ferror;
@@ -385,8 +406,7 @@ static int create=5Factive(struct sock=5Fmapping *map, =
int *evtchn)
 out=5Ferror:
 	if (*evtchn >=3D 0)
 		xenbus=5Ffree=5Fevtchn(pvcalls=5Ffront=5Fdev, *evtchn);
-	free=5Fpages((unsigned long)map->active.data.in, PVCALLS=5FRING=5FORDER);
-	free=5Fpage((unsigned long)map->active.ring);
+	free=5Factive=5Fring(map);
 	return ret;
 }
=20
@@ -406,11 +426,17 @@ int pvcalls=5Ffront=5Fconnect(struct socket *sock, st=
ruct sockaddr *addr,
 		return PTR=5FERR(map);
=20
 	bedata =3D dev=5Fget=5Fdrvdata(&pvcalls=5Ffront=5Fdev->dev);
+	ret =3D alloc=5Factive=5Fring(map);
+	if (ret < 0) {
+		pvcalls=5Fexit=5Fsock(sock);
+		return ret;
+	}
=20
 	spin=5Flock(&bedata->socket=5Flock);
 	ret =3D get=5Frequest(bedata, &req=5Fid);
 	if (ret < 0) {
 		spin=5Funlock(&bedata->socket=5Flock);
+		free=5Factive=5Fring(map);
 		pvcalls=5Fexit=5Fsock(sock);
 		return ret;
 	}
@@ -780,12 +806,20 @@ int pvcalls=5Ffront=5Faccept(struct socket *sock, str=
uct socket *newsock, int flags)
 		}
 	}
=20
+	ret =3D alloc=5Factive=5Fring(map);
+	if (ret < 0) {
+		clear=5Fbit(PVCALLS=5FFLAG=5FACCEPT=5FINFLIGHT,
+				(void *)&map->passive.flags);
+		pvcalls=5Fexit=5Fsock(sock);
+		return ret;
+	}
 	spin=5Flock(&bedata->socket=5Flock);
 	ret =3D get=5Frequest(bedata, &req=5Fid);
 	if (ret < 0) {
 		clear=5Fbit(PVCALLS=5FFLAG=5FACCEPT=5FINFLIGHT,
 			  (void *)&map->passive.flags);
 		spin=5Funlock(&bedata->socket=5Flock);
+		free=5Factive=5Fring(map);
 		pvcalls=5Fexit=5Fsock(sock);
 		return ret;
 	}
@@ -794,6 +828,7 @@ int pvcalls=5Ffront=5Faccept(struct socket *sock, struc=
t socket *newsock, int flags)
 		clear=5Fbit(PVCALLS=5FFLAG=5FACCEPT=5FINFLIGHT,
 			  (void *)&map->passive.flags);
 		spin=5Funlock(&bedata->socket=5Flock);
+		free=5Factive=5Fring(map);
 		pvcalls=5Fexit=5Fsock(sock);
 		return -ENOMEM;
 	}
--=20
2.19.1