On 11/30/18 6:01 AM, Wen Yang wrote: > The problem is that we call this with a spin lock held. > The call tree is: > pvcalls_front_accept() holds bedata->socket_lock. > -> create_active() > -> __get_free_pages() uses GFP_KERNEL > > The create_active() function is only called from pvcalls_front_accept() > with a spin_lock held, The allocation is not allowed to sleep and > GFP_KERNEL 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_active(). > v3: Use the matching deallocators i.e., free_page() > and free_pages(), respectively. > > Suggested-by: Juergen Gross <[email protected]> > Signed-off-by: Wen Yang <[email protected]> > CC: Julia Lawall <[email protected]> > CC: Boris Ostrovsky <[email protected]> > CC: Juergen Gross <[email protected]> > CC: Stefano Stabellini <[email protected]> > CC: [email protected] > CC: [email protected] > --- > drivers/xen/pvcalls-front.c | 71 ++++++++++++++++++++++++++++++------- > 1 file changed, 59 insertions(+), 12 deletions(-) > > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c > index 2f11ca72a281..a26f416daf46 100644 > --- a/drivers/xen/pvcalls-front.c > +++ b/drivers/xen/pvcalls-front.c > @@ -335,7 +335,43 @@ int pvcalls_front_socket(struct socket *sock) > return ret; > } > > -static int create_active(struct sock_mapping *map, int *evtchn) > +struct sock_mapping_active_ring { > + struct pvcalls_data_intf *ring; > + RING_IDX ring_order; > + void *bytes; > +}; > + > +static int alloc_active_ring(struct sock_mapping_active_ring *active_ring) > +{ > + active_ring->ring = NULL;
This is not necessary. > + active_ring->bytes = NULL; > + > + active_ring->ring = (struct pvcalls_data_intf *) > + __get_free_page(GFP_KERNEL | __GFP_ZERO); > + if (active_ring->ring == NULL) > + goto out_error; > + active_ring->ring_order = PVCALLS_RING_ORDER; > + active_ring->bytes = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, > + PVCALLS_RING_ORDER); > + if (active_ring->bytes == NULL) > + goto out_error; > + > + return 0; > + > +out_error: > + free_pages((unsigned long)active_ring->bytes, active_ring->ring_order); > + free_page((unsigned long)active_ring->ring); > + return -ENOMEM; > +} > + > @@ -397,6 +427,7 @@ int pvcalls_front_connect(struct socket *sock, struct > sockaddr *addr, > struct sock_mapping *map = NULL; > struct xen_pvcalls_request *req; > int notify, req_id, ret, evtchn; > + struct sock_mapping_active_ring active_ring; > > if (addr->sa_family != AF_INET || sock->type != SOCK_STREAM) > return -EOPNOTSUPP; > @@ -406,15 +437,21 @@ int pvcalls_front_connect(struct socket *sock, struct > sockaddr *addr, > return PTR_ERR(map); > > bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > + ret = alloc_active_ring(&active_ring); Why not just alloc_active_ring(map)? -boris _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
