| Message ID | 20200108152100.7630-5-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | basic KASAN support for Xen PV domains | expand |
On 1/8/20 4:21 PM, Sergey Dyasli wrote: > From: Ross Lagerwall <ross.lagerwall@citrix.com> > > When KASAN (or SLUB_DEBUG) is turned on, the normal expectation that > allocations are aligned to the next power of 2 of the size does not > hold. Hmm, really? They should after 59bb47985c1d ("mm, sl[aou]b: guarantee natural alignment for kmalloc(power-of-two)"), i.e. since 5.4. But actually the guarantee is only for precise power of two sizes given to kmalloc(). Allocations of sizes that end up using the 96 or 192 bytes kmalloc cache have no such guarantee. But those might then cross page boundary also without SLUB_DEBUG. > Therefore, handle grant copies that cross page boundaries. > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > --- > RFC --> v1: > - Added BUILD_BUG_ON to the netback patch > - xenvif_idx_release() now located outside the loop > > CC: Wei Liu <wei.liu@kernel.org> > CC: Paul Durrant <paul@xen.org> > --- > drivers/net/xen-netback/common.h | 2 +- > drivers/net/xen-netback/netback.c | 59 +++++++++++++++++++++++++------ > 2 files changed, 49 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h > index 05847eb91a1b..e57684415edd 100644 > --- a/drivers/net/xen-netback/common.h > +++ b/drivers/net/xen-netback/common.h > @@ -155,7 +155,7 @@ struct xenvif_queue { /* Per-queue data for xenvif */ > struct pending_tx_info pending_tx_info[MAX_PENDING_REQS]; > grant_handle_t grant_tx_handle[MAX_PENDING_REQS]; > > - struct gnttab_copy tx_copy_ops[MAX_PENDING_REQS]; > + struct gnttab_copy tx_copy_ops[MAX_PENDING_REQS * 2]; > struct gnttab_map_grant_ref tx_map_ops[MAX_PENDING_REQS]; > struct gnttab_unmap_grant_ref tx_unmap_ops[MAX_PENDING_REQS]; > /* passed to gnttab_[un]map_refs with pages under (un)mapping */ > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c > index 0020b2e8c279..33b8f8d043e6 100644 > --- a/drivers/net/xen-netback/netback.c > +++ b/drivers/net/xen-netback/netback.c > @@ -320,6 +320,7 @@ static int xenvif_count_requests(struct xenvif_queue *queue, > > struct xenvif_tx_cb { > u16 pending_idx; > + u8 copies; > }; > > #define XENVIF_TX_CB(skb) ((struct xenvif_tx_cb *)(skb)->cb) > @@ -439,6 +440,7 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue, > { > struct gnttab_map_grant_ref *gop_map = *gopp_map; > u16 pending_idx = XENVIF_TX_CB(skb)->pending_idx; > + u8 copies = XENVIF_TX_CB(skb)->copies; > /* This always points to the shinfo of the skb being checked, which > * could be either the first or the one on the frag_list > */ > @@ -450,23 +452,26 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue, > int nr_frags = shinfo->nr_frags; > const bool sharedslot = nr_frags && > frag_get_pending_idx(&shinfo->frags[0]) == pending_idx; > - int i, err; > + int i, err = 0; > > - /* Check status of header. */ > - err = (*gopp_copy)->status; > - if (unlikely(err)) { > - if (net_ratelimit()) > - netdev_dbg(queue->vif->dev, > + while (copies) { > + /* Check status of header. */ > + int newerr = (*gopp_copy)->status; > + if (unlikely(newerr)) { > + if (net_ratelimit()) > + netdev_dbg(queue->vif->dev, > "Grant copy of header failed! status: %d pending_idx: %u ref: %u\n", > (*gopp_copy)->status, > pending_idx, > (*gopp_copy)->source.u.ref); > - /* The first frag might still have this slot mapped */ > - if (!sharedslot) > - xenvif_idx_release(queue, pending_idx, > - XEN_NETIF_RSP_ERROR); > + err = newerr; > + } > + (*gopp_copy)++; > + copies--; > } > - (*gopp_copy)++; > + /* The first frag might still have this slot mapped */ > + if (unlikely(err) && !sharedslot) > + xenvif_idx_release(queue, pending_idx, XEN_NETIF_RSP_ERROR); > > check_frags: > for (i = 0; i < nr_frags; i++, gop_map++) { > @@ -910,6 +915,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue, > xenvif_tx_err(queue, &txreq, extra_count, idx); > break; > } > + XENVIF_TX_CB(skb)->copies = 0; > > skb_shinfo(skb)->nr_frags = ret; > if (data_len < txreq.size) > @@ -933,6 +939,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue, > "Can't allocate the frag_list skb.\n"); > break; > } > + XENVIF_TX_CB(nskb)->copies = 0; > } > > if (extras[XEN_NETIF_EXTRA_TYPE_GSO - 1].type) { > @@ -990,6 +997,31 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue, > > queue->tx_copy_ops[*copy_ops].len = data_len; > queue->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref; > + XENVIF_TX_CB(skb)->copies++; > + > + if (offset_in_page(skb->data) + data_len > XEN_PAGE_SIZE) { > + unsigned int extra_len = offset_in_page(skb->data) + > + data_len - XEN_PAGE_SIZE; > + > + queue->tx_copy_ops[*copy_ops].len -= extra_len; > + (*copy_ops)++; > + > + queue->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref; > + queue->tx_copy_ops[*copy_ops].source.domid = > + queue->vif->domid; > + queue->tx_copy_ops[*copy_ops].source.offset = > + txreq.offset + data_len - extra_len; > + > + queue->tx_copy_ops[*copy_ops].dest.u.gmfn = > + virt_to_gfn(skb->data + data_len - extra_len); > + queue->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF; > + queue->tx_copy_ops[*copy_ops].dest.offset = 0; > + > + queue->tx_copy_ops[*copy_ops].len = extra_len; > + queue->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref; > + > + XENVIF_TX_CB(skb)->copies++; > + } > > (*copy_ops)++; > > @@ -1674,5 +1706,10 @@ static void __exit netback_fini(void) > } > module_exit(netback_fini); > > +static void __init __maybe_unused build_assertions(void) > +{ > + BUILD_BUG_ON(sizeof(struct xenvif_tx_cb) > 48); > +} > + > MODULE_LICENSE("Dual BSD/GPL"); > MODULE_ALIAS("xen-backend:vif"); >
On Wed, 8 Jan 2020 at 15:21, Sergey Dyasli <sergey.dyasli@citrix.com> wrote: > > From: Ross Lagerwall <ross.lagerwall@citrix.com> > > When KASAN (or SLUB_DEBUG) is turned on, the normal expectation that > allocations are aligned to the next power of 2 of the size does not > hold. Therefore, handle grant copies that cross page boundaries. > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> > --- > RFC --> v1: > - Added BUILD_BUG_ON to the netback patch > - xenvif_idx_release() now located outside the loop > > CC: Wei Liu <wei.liu@kernel.org> > CC: Paul Durrant <paul@xen.org> [snip] > > +static void __init __maybe_unused build_assertions(void) > +{ > + BUILD_BUG_ON(sizeof(struct xenvif_tx_cb) > 48); FIELD_SIZEOF(struct sk_buff, cb) rather than a magic '48' I think. Paul > +} > + > MODULE_LICENSE("Dual BSD/GPL"); > MODULE_ALIAS("xen-backend:vif"); > -- > 2.17.1 >
On 09/01/2020 13:36, Paul Durrant wrote: > On Wed, 8 Jan 2020 at 15:21, Sergey Dyasli <sergey.dyasli@citrix.com> wrote: >> >> From: Ross Lagerwall <ross.lagerwall@citrix.com> >> >> When KASAN (or SLUB_DEBUG) is turned on, the normal expectation that >> allocations are aligned to the next power of 2 of the size does not >> hold. Therefore, handle grant copies that cross page boundaries. >> >> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> >> --- >> RFC --> v1: >> - Added BUILD_BUG_ON to the netback patch >> - xenvif_idx_release() now located outside the loop >> >> CC: Wei Liu <wei.liu@kernel.org> >> CC: Paul Durrant <paul@xen.org> > [snip] >> >> +static void __init __maybe_unused build_assertions(void) >> +{ >> + BUILD_BUG_ON(sizeof(struct xenvif_tx_cb) > 48); > > FIELD_SIZEOF(struct sk_buff, cb) rather than a magic '48' I think. The macro got renamed recently, so now it should be: sizeof_field(struct sk_buff, cb)) Thanks for the suggestion. -- Sergey
On 09/01/2020 10:33, Vlastimil Babka wrote: > On 1/8/20 4:21 PM, Sergey Dyasli wrote: >> From: Ross Lagerwall <ross.lagerwall@citrix.com> >> >> When KASAN (or SLUB_DEBUG) is turned on, the normal expectation that >> allocations are aligned to the next power of 2 of the size does not >> hold. > > Hmm, really? They should after 59bb47985c1d ("mm, sl[aou]b: guarantee > natural alignment for kmalloc(power-of-two)"), i.e. since 5.4. > > But actually the guarantee is only for precise power of two sizes given > to kmalloc(). Allocations of sizes that end up using the 96 or 192 bytes > kmalloc cache have no such guarantee. But those might then cross page > boundary also without SLUB_DEBUG. That's interesting to know. It's certainly not the case for 4.19 kernel for which PV KASAN was initially developed. But I guess this means that only patch description needs updating. > >> Therefore, handle grant copies that cross page boundaries. >> >> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> -- Thanks, Sergey
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h index 05847eb91a1b..e57684415edd 100644 --- a/drivers/net/xen-netback/common.h +++ b/drivers/net/xen-netback/common.h @@ -155,7 +155,7 @@ struct xenvif_queue { /* Per-queue data for xenvif */ struct pending_tx_info pending_tx_info[MAX_PENDING_REQS]; grant_handle_t grant_tx_handle[MAX_PENDING_REQS]; - struct gnttab_copy tx_copy_ops[MAX_PENDING_REQS]; + struct gnttab_copy tx_copy_ops[MAX_PENDING_REQS * 2]; struct gnttab_map_grant_ref tx_map_ops[MAX_PENDING_REQS]; struct gnttab_unmap_grant_ref tx_unmap_ops[MAX_PENDING_REQS]; /* passed to gnttab_[un]map_refs with pages under (un)mapping */ diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index 0020b2e8c279..33b8f8d043e6 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -320,6 +320,7 @@ static int xenvif_count_requests(struct xenvif_queue *queue, struct xenvif_tx_cb { u16 pending_idx; + u8 copies; }; #define XENVIF_TX_CB(skb) ((struct xenvif_tx_cb *)(skb)->cb) @@ -439,6 +440,7 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue, { struct gnttab_map_grant_ref *gop_map = *gopp_map; u16 pending_idx = XENVIF_TX_CB(skb)->pending_idx; + u8 copies = XENVIF_TX_CB(skb)->copies; /* This always points to the shinfo of the skb being checked, which * could be either the first or the one on the frag_list */ @@ -450,23 +452,26 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue, int nr_frags = shinfo->nr_frags; const bool sharedslot = nr_frags && frag_get_pending_idx(&shinfo->frags[0]) == pending_idx; - int i, err; + int i, err = 0; - /* Check status of header. */ - err = (*gopp_copy)->status; - if (unlikely(err)) { - if (net_ratelimit()) - netdev_dbg(queue->vif->dev, + while (copies) { + /* Check status of header. */ + int newerr = (*gopp_copy)->status; + if (unlikely(newerr)) { + if (net_ratelimit()) + netdev_dbg(queue->vif->dev, "Grant copy of header failed! status: %d pending_idx: %u ref: %u\n", (*gopp_copy)->status, pending_idx, (*gopp_copy)->source.u.ref); - /* The first frag might still have this slot mapped */ - if (!sharedslot) - xenvif_idx_release(queue, pending_idx, - XEN_NETIF_RSP_ERROR); + err = newerr; + } + (*gopp_copy)++; + copies--; } - (*gopp_copy)++; + /* The first frag might still have this slot mapped */ + if (unlikely(err) && !sharedslot) + xenvif_idx_release(queue, pending_idx, XEN_NETIF_RSP_ERROR); check_frags: for (i = 0; i < nr_frags; i++, gop_map++) { @@ -910,6 +915,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue, xenvif_tx_err(queue, &txreq, extra_count, idx); break; } + XENVIF_TX_CB(skb)->copies = 0; skb_shinfo(skb)->nr_frags = ret; if (data_len < txreq.size) @@ -933,6 +939,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue, "Can't allocate the frag_list skb.\n"); break; } + XENVIF_TX_CB(nskb)->copies = 0; } if (extras[XEN_NETIF_EXTRA_TYPE_GSO - 1].type) { @@ -990,6 +997,31 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue, queue->tx_copy_ops[*copy_ops].len = data_len; queue->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref; + XENVIF_TX_CB(skb)->copies++; + + if (offset_in_page(skb->data) + data_len > XEN_PAGE_SIZE) { + unsigned int extra_len = offset_in_page(skb->data) + + data_len - XEN_PAGE_SIZE; + + queue->tx_copy_ops[*copy_ops].len -= extra_len; + (*copy_ops)++; + + queue->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref; + queue->tx_copy_ops[*copy_ops].source.domid = + queue->vif->domid; + queue->tx_copy_ops[*copy_ops].source.offset = + txreq.offset + data_len - extra_len; + + queue->tx_copy_ops[*copy_ops].dest.u.gmfn = + virt_to_gfn(skb->data + data_len - extra_len); + queue->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF; + queue->tx_copy_ops[*copy_ops].dest.offset = 0; + + queue->tx_copy_ops[*copy_ops].len = extra_len; + queue->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref; + + XENVIF_TX_CB(skb)->copies++; + } (*copy_ops)++; @@ -1674,5 +1706,10 @@ static void __exit netback_fini(void) } module_exit(netback_fini); +static void __init __maybe_unused build_assertions(void) +{ + BUILD_BUG_ON(sizeof(struct xenvif_tx_cb) > 48); +} + MODULE_LICENSE("Dual BSD/GPL"); MODULE_ALIAS("xen-backend:vif");