This series fixes a lot of bugs on the error path around this function, which
were introduced with my grant mapping series in 3.15. They apply to the latest
net tree, but probably to net-next as well without any modification.
I'll post an another series which applies to 3.15 stable, as the problem was
first discovered there. The only difference is that the "queue" variable name is
replaced to "vif".
Signed-off-by: Zoltan Kiss <[email protected]>
Reported-by: Armin Zentai <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
The error handling for skb's with frag_list was completely wrong, it caused
double unmap attempts to happen if the error was on the first skb. Move it to
the right place in the loop.
Signed-off-by: Zoltan Kiss <[email protected]>
Reported-by: Armin Zentai <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 1844a47..604ff71 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1030,10 +1030,16 @@ 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;
+ /* This points to the shinfo of the actually checked skb, which could be
+ * either the first or the one on the frag_list
+ */
struct skb_shared_info *shinfo = skb_shinfo(skb);
+ /* If this is non-NULL, we are currently checking the frag_list skb, and
+ * this points to the shinfo of the first one
+ */
+ struct skb_shared_info *first_shinfo = NULL;
int nr_frags = shinfo->nr_frags;
int i, err;
- struct sk_buff *first_skb = NULL;
/* Check status of header. */
err = (*gopp_copy)->status;
@@ -1086,31 +1092,28 @@ check_frags:
xenvif_idx_unmap(queue, pending_idx);
}
+ /* And if we found the error while checking the frag_list, unmap
+ * the first skb's frags
+ */
+ if (first_shinfo) {
+ for (j = 0; j < first_shinfo->nr_frags; j++) {
+ pending_idx = frag_get_pending_idx(&first_shinfo->frags[j]);
+ xenvif_idx_unmap(queue, pending_idx);
+ }
+ }
+
/* Remember the error: invalidate all subsequent fragments. */
err = newerr;
}
- if (skb_has_frag_list(skb)) {
- first_skb = skb;
- skb = shinfo->frag_list;
- shinfo = skb_shinfo(skb);
+ if (skb_has_frag_list(skb) && !first_shinfo) {
+ first_shinfo = skb_shinfo(skb);
+ shinfo = skb_shinfo(skb_shinfo(skb)->frag_list);
nr_frags = shinfo->nr_frags;
goto check_frags;
}
- /* There was a mapping error in the frag_list skb. We have to unmap
- * the first skb's frags
- */
- if (first_skb && err) {
- int j;
- shinfo = skb_shinfo(first_skb);
- for (j = 0; j < shinfo->nr_frags; j++) {
- pending_idx = frag_get_pending_idx(&shinfo->frags[j]);
- xenvif_idx_unmap(queue, pending_idx);
- }
- }
-
*gopp_map = gop_map;
return err;
}
When the grant operations failed, the skb is freed up eventually, and it tries
to release the frags, if there is any. For the main skb nr_frags is set to 0 to
avoid this, but on the frag_list it iterates through the frags array, and tries
to call put_page on the page pointer which contains garbage at that time.
Signed-off-by: Zoltan Kiss <[email protected]>
Reported-by: Armin Zentai <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 604ff71..e9ffb05 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1522,6 +1522,11 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
/* Check the remap error code. */
if (unlikely(xenvif_tx_check_gop(queue, skb, &gop_map, &gop_copy))) {
skb_shinfo(skb)->nr_frags = 0;
+ if (skb_has_frag_list(skb)) {
+ struct sk_buff *nskb =
+ skb_shinfo(skb)->frag_list;
+ skb_shinfo(nskb)->nr_frags = 0;
+ }
kfree_skb(skb);
continue;
}
This patch makes this function aware that the first frag and the header might
share the same ring slot. That could happen if the first slot is bigger than
MAX_SKB_LEN. Due to this the error path might release that slot twice or never,
depending on the error scenario.
xenvif_idx_release is also removed from xenvif_idx_unmap, and called separately.
Signed-off-by: Zoltan Kiss <[email protected]>
Reported-by: Armin Zentai <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index e9ffb05..938d5a9 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1039,6 +1039,8 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
*/
struct skb_shared_info *first_shinfo = NULL;
int nr_frags = shinfo->nr_frags;
+ const bool sharedslot = nr_frags &&
+ frag_get_pending_idx(&shinfo->frags[0]) == pending_idx;
int i, err;
/* Check status of header. */
@@ -1051,7 +1053,10 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
(*gopp_copy)->status,
pending_idx,
(*gopp_copy)->source.u.ref);
- xenvif_idx_release(queue, pending_idx, XEN_NETIF_RSP_ERROR);
+ /* The first frag might still have this slot mapped */
+ if (!sharedslot)
+ xenvif_idx_release(queue, pending_idx,
+ XEN_NETIF_RSP_ERROR);
}
check_frags:
@@ -1068,8 +1073,19 @@ check_frags:
pending_idx,
gop_map->handle);
/* Had a previous error? Invalidate this fragment. */
- if (unlikely(err))
+ if (unlikely(err)) {
xenvif_idx_unmap(queue, pending_idx);
+ /* If the mapping of the first frag was OK, but
+ * the header's copy failed, and they are
+ * sharing a slot, send an error
+ */
+ if (i == 0 && sharedslot)
+ xenvif_idx_release(queue, pending_idx,
+ XEN_NETIF_RSP_ERROR);
+ else
+ xenvif_idx_release(queue, pending_idx,
+ XEN_NETIF_RSP_OKAY);
+ }
continue;
}
@@ -1081,15 +1097,27 @@ check_frags:
gop_map->status,
pending_idx,
gop_map->ref);
+
xenvif_idx_release(queue, pending_idx, XEN_NETIF_RSP_ERROR);
/* Not the first error? Preceding frags already invalidated. */
if (err)
continue;
- /* First error: invalidate preceding fragments. */
+
+ /* First error: if the header haven't shared a slot with the
+ * first frag, release it as well.
+ */
+ if (!sharedslot)
+ xenvif_idx_release(queue,
+ XENVIF_TX_CB(skb)->pending_idx,
+ XEN_NETIF_RSP_OKAY);
+
+ /* Invalidate preceding fragments of this skb. */
for (j = 0; j < i; j++) {
pending_idx = frag_get_pending_idx(&shinfo->frags[j]);
xenvif_idx_unmap(queue, pending_idx);
+ xenvif_idx_release(queue, pending_idx,
+ XEN_NETIF_RSP_OKAY);
}
/* And if we found the error while checking the frag_list, unmap
@@ -1099,6 +1127,8 @@ check_frags:
for (j = 0; j < first_shinfo->nr_frags; j++) {
pending_idx = frag_get_pending_idx(&first_shinfo->frags[j]);
xenvif_idx_unmap(queue, pending_idx);
+ xenvif_idx_release(queue, pending_idx,
+ XEN_NETIF_RSP_OKAY);
}
}
@@ -1830,8 +1860,6 @@ void xenvif_idx_unmap(struct xenvif_queue *queue, u16 pending_idx)
tx_unmap_op.status);
BUG();
}
-
- xenvif_idx_release(queue, pending_idx, XEN_NETIF_RSP_OKAY);
}
static inline int rx_work_todo(struct xenvif_queue *queue)
Due to this pointer is increased prematurely, the error log contains rubbish.
Signed-off-by: Zoltan Kiss <[email protected]>
Reported-by: Armin Zentai <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index cca82de..72d8a6b 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1045,7 +1045,6 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
/* Check status of header. */
err = (*gopp_copy)->status;
- (*gopp_copy)++;
if (unlikely(err)) {
if (net_ratelimit())
netdev_dbg(queue->vif->dev,
@@ -1058,6 +1057,7 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
xenvif_idx_release(queue, pending_idx,
XEN_NETIF_RSP_ERROR);
}
+ (*gopp_copy)++;
check_frags:
for (i = 0; i < nr_frags; i++, gop_map++) {
On Thu, Jul 17, 2014 at 08:09:49PM +0100, Zoltan Kiss wrote:
> The error handling for skb's with frag_list was completely wrong, it caused
> double unmap attempts to happen if the error was on the first skb. Move it to
> the right place in the loop.
>
> Signed-off-by: Zoltan Kiss <[email protected]>
> Reported-by: Armin Zentai <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 1844a47..604ff71 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1030,10 +1030,16 @@ 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;
> + /* This points to the shinfo of the actually checked skb, which could be
> + * either the first or the one on the frag_list
> + */
I think "checked skb" should be "skb being checked". Feel free to
disagree as I'm not native English speaker. :-/
> struct skb_shared_info *shinfo = skb_shinfo(skb);
> + /* If this is non-NULL, we are currently checking the frag_list skb, and
> + * this points to the shinfo of the first one
> + */
> + struct skb_shared_info *first_shinfo = NULL;
> int nr_frags = shinfo->nr_frags;
> int i, err;
> - struct sk_buff *first_skb = NULL;
>
> /* Check status of header. */
> err = (*gopp_copy)->status;
> @@ -1086,31 +1092,28 @@ check_frags:
> xenvif_idx_unmap(queue, pending_idx);
> }
>
> + /* And if we found the error while checking the frag_list, unmap
> + * the first skb's frags
> + */
> + if (first_shinfo) {
> + for (j = 0; j < first_shinfo->nr_frags; j++) {
> + pending_idx = frag_get_pending_idx(&first_shinfo->frags[j]);
> + xenvif_idx_unmap(queue, pending_idx);
> + }
> + }
> +
> /* Remember the error: invalidate all subsequent fragments. */
> err = newerr;
> }
>
> - if (skb_has_frag_list(skb)) {
> - first_skb = skb;
> - skb = shinfo->frag_list;
> - shinfo = skb_shinfo(skb);
> + if (skb_has_frag_list(skb) && !first_shinfo) {
Will it ever come to the point that we have another skb in this skb's
frag list? Is there any reason prevents you from looping over the
(possible) subsequent skbs? I guess if the error is deep in the list
it's a bit hard to bookkeep...
> + first_shinfo = skb_shinfo(skb);
> + shinfo = skb_shinfo(skb_shinfo(skb)->frag_list);
In that case I would suggest you add
BUG_ON(skb_has_frag_list(skb_shinfo(skb)->frag_list)). I think having
more nested frag_list should be a bug in current design.
Wei.
On Thu, Jul 17, 2014 at 08:09:50PM +0100, Zoltan Kiss wrote:
> When the grant operations failed, the skb is freed up eventually, and it tries
> to release the frags, if there is any. For the main skb nr_frags is set to 0 to
> avoid this, but on the frag_list it iterates through the frags array, and tries
> to call put_page on the page pointer which contains garbage at that time.
>
> Signed-off-by: Zoltan Kiss <[email protected]>
> Reported-by: Armin Zentai <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 604ff71..e9ffb05 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1522,6 +1522,11 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> /* Check the remap error code. */
> if (unlikely(xenvif_tx_check_gop(queue, skb, &gop_map, &gop_copy))) {
It's worth adding a comment here saying that all those pages have been
freed in xenvif_tx_check_gop so that we don't leak any page even if we
manually set nr_frags to 0.
Wei.
> skb_shinfo(skb)->nr_frags = 0;
> + if (skb_has_frag_list(skb)) {
> + struct sk_buff *nskb =
> + skb_shinfo(skb)->frag_list;
> + skb_shinfo(nskb)->nr_frags = 0;
> + }
> kfree_skb(skb);
> continue;
> }
On Thu, Jul 17, 2014 at 08:09:51PM +0100, Zoltan Kiss wrote:
> This patch makes this function aware that the first frag and the header might
> share the same ring slot. That could happen if the first slot is bigger than
> MAX_SKB_LEN. Due to this the error path might release that slot twice or never,
I guess you mean PKT_PROT_LEN.
Just one question, how come that we didn't come across this with copying
backend? Comparing txreq.size against PKT_PROT_LEN is not new in mapping
backend.
> depending on the error scenario.
> xenvif_idx_release is also removed from xenvif_idx_unmap, and called separately.
>
> Signed-off-by: Zoltan Kiss <[email protected]>
> Reported-by: Armin Zentai <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index e9ffb05..938d5a9 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1039,6 +1039,8 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
> */
> struct skb_shared_info *first_shinfo = NULL;
> int nr_frags = shinfo->nr_frags;
> + const bool sharedslot = nr_frags &&
> + frag_get_pending_idx(&shinfo->frags[0]) == pending_idx;
> int i, err;
>
> /* Check status of header. */
> @@ -1051,7 +1053,10 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
> (*gopp_copy)->status,
> pending_idx,
> (*gopp_copy)->source.u.ref);
> - xenvif_idx_release(queue, pending_idx, XEN_NETIF_RSP_ERROR);
> + /* The first frag might still have this slot mapped */
> + if (!sharedslot)
> + xenvif_idx_release(queue, pending_idx,
> + XEN_NETIF_RSP_ERROR);
> }
>
> check_frags:
> @@ -1068,8 +1073,19 @@ check_frags:
> pending_idx,
> gop_map->handle);
> /* Had a previous error? Invalidate this fragment. */
> - if (unlikely(err))
> + if (unlikely(err)) {
> xenvif_idx_unmap(queue, pending_idx);
> + /* If the mapping of the first frag was OK, but
> + * the header's copy failed, and they are
> + * sharing a slot, send an error
> + */
> + if (i == 0 && sharedslot)
> + xenvif_idx_release(queue, pending_idx,
> + XEN_NETIF_RSP_ERROR);
> + else
> + xenvif_idx_release(queue, pending_idx,
> + XEN_NETIF_RSP_OKAY);
I guess this is sort of OK, just a bit fragile. Couldn't think of a
better way to refactor this function. :-(
> + }
> continue;
> }
>
> @@ -1081,15 +1097,27 @@ check_frags:
> gop_map->status,
> pending_idx,
> gop_map->ref);
> +
Stray blank line.
And did you miss a callsite of xenvif_idx_unmap in this function which
is added in your first patch?
Wei.
On Thu, Jul 17, 2014 at 08:09:52PM +0100, Zoltan Kiss wrote:
> Due to this pointer is increased prematurely, the error log contains rubbish.
>
> Signed-off-by: Zoltan Kiss <[email protected]>
> Reported-by: Armin Zentai <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Acked-by: Wei Liu <[email protected]>
On 18/07/14 16:24, Wei Liu wrote:
> On Thu, Jul 17, 2014 at 08:09:49PM +0100, Zoltan Kiss wrote:
>> The error handling for skb's with frag_list was completely wrong, it caused
>> double unmap attempts to happen if the error was on the first skb. Move it to
>> the right place in the loop.
>>
>> Signed-off-by: Zoltan Kiss <[email protected]>
>> Reported-by: Armin Zentai <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>> index 1844a47..604ff71 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -1030,10 +1030,16 @@ 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;
>> + /* This points to the shinfo of the actually checked skb, which could be
>> + * either the first or the one on the frag_list
>> + */
>
> I think "checked skb" should be "skb being checked". Feel free to
> disagree as I'm not native English speaker. :-/
>
>> struct skb_shared_info *shinfo = skb_shinfo(skb);
>> + /* If this is non-NULL, we are currently checking the frag_list skb, and
>> + * this points to the shinfo of the first one
>> + */
>> + struct skb_shared_info *first_shinfo = NULL;
>> int nr_frags = shinfo->nr_frags;
>> int i, err;
>> - struct sk_buff *first_skb = NULL;
>>
>> /* Check status of header. */
>> err = (*gopp_copy)->status;
>> @@ -1086,31 +1092,28 @@ check_frags:
>> xenvif_idx_unmap(queue, pending_idx);
>> }
>>
>> + /* And if we found the error while checking the frag_list, unmap
>> + * the first skb's frags
>> + */
>> + if (first_shinfo) {
>> + for (j = 0; j < first_shinfo->nr_frags; j++) {
>> + pending_idx = frag_get_pending_idx(&first_shinfo->frags[j]);
>> + xenvif_idx_unmap(queue, pending_idx);
>> + }
>> + }
>> +
>> /* Remember the error: invalidate all subsequent fragments. */
>> err = newerr;
>> }
>>
>> - if (skb_has_frag_list(skb)) {
>> - first_skb = skb;
>> - skb = shinfo->frag_list;
>> - shinfo = skb_shinfo(skb);
>> + if (skb_has_frag_list(skb) && !first_shinfo) {
>
> Will it ever come to the point that we have another skb in this skb's
> frag list? Is there any reason prevents you from looping over the
> (possible) subsequent skbs? I guess if the error is deep in the list
> it's a bit hard to bookkeep...
>
>> + first_shinfo = skb_shinfo(skb);
>> + shinfo = skb_shinfo(skb_shinfo(skb)->frag_list);
>
> In that case I would suggest you add
> BUG_ON(skb_has_frag_list(skb_shinfo(skb)->frag_list)). I think having
> more nested frag_list should be a bug in current design.
There are already 3 things which prevents this
- in count_requests we drop the packet if it has more than
XEN_NETBK_LEGACY_SLOTS_MAX slots
- in get_requests there is a BUG_ON(frag_overflow > MAX_SKB_FRAGS),
which shouldn't really due to the prev point
- in the same funciont we create a frag_list skb exactly once
On 18/07/14 16:25, Wei Liu wrote:
> On Thu, Jul 17, 2014 at 08:09:51PM +0100, Zoltan Kiss wrote:
>> This patch makes this function aware that the first frag and the header might
>> share the same ring slot. That could happen if the first slot is bigger than
>> MAX_SKB_LEN. Due to this the error path might release that slot twice or never,
>
> I guess you mean PKT_PROT_LEN.
Yes
>
> Just one question, how come that we didn't come across this with copying
> backend? Comparing txreq.size against PKT_PROT_LEN is not new in mapping
> backend.
We had one grant copy for the header and the first frag in that case,
and we skipped the first frag:
/* Skip first skb fragment if it is on same page as header fragment. */
start = (frag_get_pending_idx(&shinfo->frags[0]) == pending_idx);
>
>> depending on the error scenario.
>> xenvif_idx_release is also removed from xenvif_idx_unmap, and called separately.
>>
>> Signed-off-by: Zoltan Kiss <[email protected]>
>> Reported-by: Armin Zentai <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>> index e9ffb05..938d5a9 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -1039,6 +1039,8 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
>> */
>> struct skb_shared_info *first_shinfo = NULL;
>> int nr_frags = shinfo->nr_frags;
>> + const bool sharedslot = nr_frags &&
>> + frag_get_pending_idx(&shinfo->frags[0]) == pending_idx;
>> int i, err;
>>
>> /* Check status of header. */
>> @@ -1051,7 +1053,10 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
>> (*gopp_copy)->status,
>> pending_idx,
>> (*gopp_copy)->source.u.ref);
>> - xenvif_idx_release(queue, pending_idx, XEN_NETIF_RSP_ERROR);
>> + /* The first frag might still have this slot mapped */
>> + if (!sharedslot)
>> + xenvif_idx_release(queue, pending_idx,
>> + XEN_NETIF_RSP_ERROR);
>> }
>>
>> check_frags:
>> @@ -1068,8 +1073,19 @@ check_frags:
>> pending_idx,
>> gop_map->handle);
>> /* Had a previous error? Invalidate this fragment. */
>> - if (unlikely(err))
>> + if (unlikely(err)) {
>> xenvif_idx_unmap(queue, pending_idx);
>> + /* If the mapping of the first frag was OK, but
>> + * the header's copy failed, and they are
>> + * sharing a slot, send an error
>> + */
>> + if (i == 0 && sharedslot)
>> + xenvif_idx_release(queue, pending_idx,
>> + XEN_NETIF_RSP_ERROR);
>> + else
>> + xenvif_idx_release(queue, pending_idx,
>> + XEN_NETIF_RSP_OKAY);
>
> I guess this is sort of OK, just a bit fragile. Couldn't think of a
> better way to refactor this function. :-(
I was thinking a lot about how to refactor this whole thing, but I gave
up too ...
>
>> + }
>> continue;
>> }
>>
>> @@ -1081,15 +1097,27 @@ check_frags:
>> gop_map->status,
>> pending_idx,
>> gop_map->ref);
>> +
>
> Stray blank line.
>
> And did you miss a callsite of xenvif_idx_unmap in this function which
> is added in your first patch?
Nope, the xenvif_idx_release is there
>
> Wei.
>