* [PATCH net-next V2 0/3] xen-netback: misc fixes
@ 2013-05-02 10:43 Wei Liu
2013-05-02 10:43 ` [PATCH net-next V2 1/3] xen-netback: remove redundent parameter in netbk_count_requests Wei Liu
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Wei Liu @ 2013-05-02 10:43 UTC (permalink / raw)
To: xen-devel, netdev; +Cc: ian.campbell, jbeulich, Wei Liu
This series fixes glitches spotted by Jan. Please see commit logs for details.
A third patch is added to have better names for different thresholds, suggested
by Ian Campbell.
Wei Liu (3):
xen-netback: remove redundent parameter in netbk_count_requests
xen-netback: avoid allocating variable size array on stack
xen-netback: better names for thresholds
drivers/net/xen-netback/netback.c | 69 +++++++++++++++++++++++--------------
1 file changed, 44 insertions(+), 25 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next V2 1/3] xen-netback: remove redundent parameter in netbk_count_requests
2013-05-02 10:43 [PATCH net-next V2 0/3] xen-netback: misc fixes Wei Liu
@ 2013-05-02 10:43 ` Wei Liu
2013-05-02 10:43 ` [PATCH net-next V2 2/3] xen-netback: avoid allocating variable size array on stack Wei Liu
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Wei Liu @ 2013-05-02 10:43 UTC (permalink / raw)
To: xen-devel, netdev; +Cc: ian.campbell, jbeulich, Wei Liu
Tracking down from the caller, first_idx is always equal to vif->tx.req_cons.
Remove it to avoid confusion.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
drivers/net/xen-netback/netback.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index a2865f1..c44772d 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -928,7 +928,6 @@ static void netbk_fatal_tx_err(struct xenvif *vif)
static int netbk_count_requests(struct xenvif *vif,
struct xen_netif_tx_request *first,
- RING_IDX first_idx,
struct xen_netif_tx_request *txp,
int work_to_do)
{
@@ -1005,7 +1004,7 @@ static int netbk_count_requests(struct xenvif *vif,
} while ((txp++)->flags & XEN_NETTXF_more_data);
if (drop_err) {
- netbk_tx_err(vif, first, first_idx + slots);
+ netbk_tx_err(vif, first, cons + slots);
return drop_err;
}
@@ -1470,8 +1469,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
continue;
}
- ret = netbk_count_requests(vif, &txreq, idx,
- txfrags, work_to_do);
+ ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do);
if (unlikely(ret < 0))
continue;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next V2 2/3] xen-netback: avoid allocating variable size array on stack
2013-05-02 10:43 [PATCH net-next V2 0/3] xen-netback: misc fixes Wei Liu
2013-05-02 10:43 ` [PATCH net-next V2 1/3] xen-netback: remove redundent parameter in netbk_count_requests Wei Liu
@ 2013-05-02 10:43 ` Wei Liu
2013-05-02 12:04 ` Jan Beulich
2013-05-02 10:43 ` [PATCH net-next V2 3/3] xen-netback: better names for thresholds Wei Liu
2013-05-02 20:50 ` [PATCH net-next V2 0/3] xen-netback: misc fixes David Miller
3 siblings, 1 reply; 8+ messages in thread
From: Wei Liu @ 2013-05-02 10:43 UTC (permalink / raw)
To: xen-devel, netdev; +Cc: ian.campbell, jbeulich, Wei Liu
Tune xen_netbk_count_requests to not touch working array beyond limit, so that
we can make working array size constant.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
drivers/net/xen-netback/netback.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index c44772d..ce8109f 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -934,11 +934,14 @@ static int netbk_count_requests(struct xenvif *vif,
RING_IDX cons = vif->tx.req_cons;
int slots = 0;
int drop_err = 0;
+ int more_data;
if (!(first->flags & XEN_NETTXF_more_data))
return 0;
do {
+ struct xen_netif_tx_request dropped_tx = { 0 };
+
if (slots >= work_to_do) {
netdev_err(vif->dev,
"Asked for %d slots but exceeds this limit\n",
@@ -972,6 +975,9 @@ static int netbk_count_requests(struct xenvif *vif,
drop_err = -E2BIG;
}
+ if (drop_err)
+ txp = &dropped_tx;
+
memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
sizeof(*txp));
@@ -1001,7 +1007,13 @@ static int netbk_count_requests(struct xenvif *vif,
netbk_fatal_tx_err(vif);
return -EINVAL;
}
- } while ((txp++)->flags & XEN_NETTXF_more_data);
+
+ more_data = txp->flags & XEN_NETTXF_more_data;
+
+ if (!drop_err)
+ txp++;
+
+ } while (more_data);
if (drop_err) {
netbk_tx_err(vif, first, cons + slots);
@@ -1408,7 +1420,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
!list_empty(&netbk->net_schedule_list)) {
struct xenvif *vif;
struct xen_netif_tx_request txreq;
- struct xen_netif_tx_request txfrags[max_skb_slots];
+ struct xen_netif_tx_request txfrags[XEN_NETIF_NR_SLOTS_MIN];
struct page *page;
struct xen_netif_extra_info extras[XEN_NETIF_EXTRA_TYPE_MAX-1];
u16 pending_idx;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next V2 3/3] xen-netback: better names for thresholds
2013-05-02 10:43 [PATCH net-next V2 0/3] xen-netback: misc fixes Wei Liu
2013-05-02 10:43 ` [PATCH net-next V2 1/3] xen-netback: remove redundent parameter in netbk_count_requests Wei Liu
2013-05-02 10:43 ` [PATCH net-next V2 2/3] xen-netback: avoid allocating variable size array on stack Wei Liu
@ 2013-05-02 10:43 ` Wei Liu
2013-05-02 20:50 ` [PATCH net-next V2 0/3] xen-netback: misc fixes David Miller
3 siblings, 0 replies; 8+ messages in thread
From: Wei Liu @ 2013-05-02 10:43 UTC (permalink / raw)
To: xen-devel, netdev; +Cc: ian.campbell, jbeulich, Wei Liu
This patch only changes some names to avoid confusion.
In this patch we have:
MAX_SKB_SLOTS_DEFAULT -> FATAL_SKB_SLOTS_DEFAULT
max_skb_slots -> fatal_skb_slots
#define XEN_NETBK_LEGACY_SLOTS_MAX XEN_NETIF_NR_SLOTS_MIN
The fatal_skb_slots is the threshold to determine whether a packet is
malicious.
XEN_NETBK_LEGACY_SLOTS_MAX is the maximum slots a valid packet can have at
this point. It is defined to be XEN_NETIF_NR_SLOTS_MIN because that's
guaranteed to be supported by all backends.
Suggested-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
drivers/net/xen-netback/netback.c | 49 ++++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index ce8109f..37984e6 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -51,9 +51,17 @@
* This is the maximum slots a skb can have. If a guest sends a skb
* which exceeds this limit it is considered malicious.
*/
-#define MAX_SKB_SLOTS_DEFAULT 20
-static unsigned int max_skb_slots = MAX_SKB_SLOTS_DEFAULT;
-module_param(max_skb_slots, uint, 0444);
+#define FATAL_SKB_SLOTS_DEFAULT 20
+static unsigned int fatal_skb_slots = FATAL_SKB_SLOTS_DEFAULT;
+module_param(fatal_skb_slots, uint, 0444);
+
+/*
+ * To avoid confusion, we define XEN_NETBK_LEGACY_SLOTS_MAX indicating
+ * the maximum slots a valid packet can use. Now this value is defined
+ * to be XEN_NETIF_NR_SLOTS_MIN, which is supposed to be supported by
+ * all backend.
+ */
+#define XEN_NETBK_LEGACY_SLOTS_MAX XEN_NETIF_NR_SLOTS_MIN
typedef unsigned int pending_ring_idx_t;
#define INVALID_PENDING_RING_IDX (~0U)
@@ -953,25 +961,26 @@ static int netbk_count_requests(struct xenvif *vif,
/* This guest is really using too many slots and
* considered malicious.
*/
- if (unlikely(slots >= max_skb_slots)) {
+ if (unlikely(slots >= fatal_skb_slots)) {
netdev_err(vif->dev,
"Malicious frontend using %d slots, threshold %u\n",
- slots, max_skb_slots);
+ slots, fatal_skb_slots);
netbk_fatal_tx_err(vif);
return -E2BIG;
}
/* Xen network protocol had implicit dependency on
- * MAX_SKB_FRAGS. XEN_NETIF_NR_SLOTS_MIN is set to the
- * historical MAX_SKB_FRAGS value 18 to honor the same
- * behavior as before. Any packet using more than 18
- * slots but less than max_skb_slots slots is dropped
+ * MAX_SKB_FRAGS. XEN_NETBK_LEGACY_SLOTS_MAX is set to
+ * the historical MAX_SKB_FRAGS value 18 to honor the
+ * same behavior as before. Any packet using more than
+ * 18 slots but less than fatal_skb_slots slots is
+ * dropped
*/
- if (!drop_err && slots >= XEN_NETIF_NR_SLOTS_MIN) {
+ if (!drop_err && slots >= XEN_NETBK_LEGACY_SLOTS_MAX) {
if (net_ratelimit())
netdev_dbg(vif->dev,
"Too many slots (%d) exceeding limit (%d), dropping packet\n",
- slots, XEN_NETIF_NR_SLOTS_MIN);
+ slots, XEN_NETBK_LEGACY_SLOTS_MAX);
drop_err = -E2BIG;
}
@@ -1053,7 +1062,7 @@ static struct gnttab_copy *xen_netbk_get_requests(struct xen_netbk *netbk,
struct pending_tx_info *first = NULL;
/* At this point shinfo->nr_frags is in fact the number of
- * slots, which can be as large as XEN_NETIF_NR_SLOTS_MIN.
+ * slots, which can be as large as XEN_NETBK_LEGACY_SLOTS_MAX.
*/
nr_slots = shinfo->nr_frags;
@@ -1415,12 +1424,12 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
struct sk_buff *skb;
int ret;
- while ((nr_pending_reqs(netbk) + XEN_NETIF_NR_SLOTS_MIN
+ while ((nr_pending_reqs(netbk) + XEN_NETBK_LEGACY_SLOTS_MAX
< MAX_PENDING_REQS) &&
!list_empty(&netbk->net_schedule_list)) {
struct xenvif *vif;
struct xen_netif_tx_request txreq;
- struct xen_netif_tx_request txfrags[XEN_NETIF_NR_SLOTS_MIN];
+ struct xen_netif_tx_request txfrags[XEN_NETBK_LEGACY_SLOTS_MAX];
struct page *page;
struct xen_netif_extra_info extras[XEN_NETIF_EXTRA_TYPE_MAX-1];
u16 pending_idx;
@@ -1508,7 +1517,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
pending_idx = netbk->pending_ring[index];
data_len = (txreq.size > PKT_PROT_LEN &&
- ret < XEN_NETIF_NR_SLOTS_MIN) ?
+ ret < XEN_NETBK_LEGACY_SLOTS_MAX) ?
PKT_PROT_LEN : txreq.size;
skb = alloc_skb(data_len + NET_SKB_PAD + NET_IP_ALIGN,
@@ -1787,7 +1796,7 @@ static inline int rx_work_todo(struct xen_netbk *netbk)
static inline int tx_work_todo(struct xen_netbk *netbk)
{
- if ((nr_pending_reqs(netbk) + XEN_NETIF_NR_SLOTS_MIN
+ if ((nr_pending_reqs(netbk) + XEN_NETBK_LEGACY_SLOTS_MAX
< MAX_PENDING_REQS) &&
!list_empty(&netbk->net_schedule_list))
return 1;
@@ -1872,11 +1881,11 @@ static int __init netback_init(void)
if (!xen_domain())
return -ENODEV;
- if (max_skb_slots < XEN_NETIF_NR_SLOTS_MIN) {
+ if (fatal_skb_slots < XEN_NETBK_LEGACY_SLOTS_MAX) {
printk(KERN_INFO
- "xen-netback: max_skb_slots too small (%d), bump it to XEN_NETIF_NR_SLOTS_MIN (%d)\n",
- max_skb_slots, XEN_NETIF_NR_SLOTS_MIN);
- max_skb_slots = XEN_NETIF_NR_SLOTS_MIN;
+ "xen-netback: fatal_skb_slots too small (%d), bump it to XEN_NETBK_LEGACY_SLOTS_MAX (%d)\n",
+ fatal_skb_slots, XEN_NETBK_LEGACY_SLOTS_MAX);
+ fatal_skb_slots = XEN_NETBK_LEGACY_SLOTS_MAX;
}
xen_netbk_group_nr = num_online_cpus();
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next V2 2/3] xen-netback: avoid allocating variable size array on stack
2013-05-02 10:43 ` [PATCH net-next V2 2/3] xen-netback: avoid allocating variable size array on stack Wei Liu
@ 2013-05-02 12:04 ` Jan Beulich
2013-05-02 15:55 ` Ian Campbell
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2013-05-02 12:04 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.campbell, xen-devel, netdev
>>> On 02.05.13 at 12:43, Wei Liu <wei.liu2@citrix.com> wrote:
> @@ -934,11 +934,14 @@ static int netbk_count_requests(struct xenvif *vif,
> RING_IDX cons = vif->tx.req_cons;
> int slots = 0;
> int drop_err = 0;
> + int more_data;
>
> if (!(first->flags & XEN_NETTXF_more_data))
> return 0;
>
> do {
> + struct xen_netif_tx_request dropped_tx = { 0 };
> +
No need for an initializer here.
> if (slots >= work_to_do) {
> netdev_err(vif->dev,
> "Asked for %d slots but exceeds this limit\n",
> @@ -972,6 +975,9 @@ static int netbk_count_requests(struct xenvif *vif,
> drop_err = -E2BIG;
> }
>
> + if (drop_err)
> + txp = &dropped_tx;
> +
> memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
> sizeof(*txp));
>
> @@ -1001,7 +1007,13 @@ static int netbk_count_requests(struct xenvif *vif,
> netbk_fatal_tx_err(vif);
> return -EINVAL;
> }
> - } while ((txp++)->flags & XEN_NETTXF_more_data);
> +
> + more_data = txp->flags & XEN_NETTXF_more_data;
> +
> + if (!drop_err)
> + txp++;
And no need for the conditional here afaict.
Jan
> +
> + } while (more_data);
>
> if (drop_err) {
> netbk_tx_err(vif, first, cons + slots);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next V2 2/3] xen-netback: avoid allocating variable size array on stack
2013-05-02 12:04 ` Jan Beulich
@ 2013-05-02 15:55 ` Ian Campbell
2013-05-02 16:00 ` Wei Liu
0 siblings, 1 reply; 8+ messages in thread
From: Ian Campbell @ 2013-05-02 15:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, xen-devel, netdev
On Thu, 2013-05-02 at 13:04 +0100, Jan Beulich wrote:
> >>> On 02.05.13 at 12:43, Wei Liu <wei.liu2@citrix.com> wrote:
> > @@ -934,11 +934,14 @@ static int netbk_count_requests(struct xenvif *vif,
> > RING_IDX cons = vif->tx.req_cons;
> > int slots = 0;
> > int drop_err = 0;
> > + int more_data;
> >
> > if (!(first->flags & XEN_NETTXF_more_data))
> > return 0;
> >
> > do {
> > + struct xen_netif_tx_request dropped_tx = { 0 };
> > +
>
> No need for an initializer here.
>
> > if (slots >= work_to_do) {
> > netdev_err(vif->dev,
> > "Asked for %d slots but exceeds this limit\n",
> > @@ -972,6 +975,9 @@ static int netbk_count_requests(struct xenvif *vif,
> > drop_err = -E2BIG;
> > }
> >
> > + if (drop_err)
> > + txp = &dropped_tx;
> > +
> > memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
> > sizeof(*txp));
> >
> > @@ -1001,7 +1007,13 @@ static int netbk_count_requests(struct xenvif *vif,
> > netbk_fatal_tx_err(vif);
> > return -EINVAL;
> > }
> > - } while ((txp++)->flags & XEN_NETTXF_more_data);
> > +
> > + more_data = txp->flags & XEN_NETTXF_more_data;
> > +
> > + if (!drop_err)
> > + txp++;
>
> And no need for the conditional here afaict.
I think it is needed, in the case where you've assigned txp =
&dropped_tx you don't want to increment txp.
Or perhaps it just gets reassigned back to &dropped_tx at the top of the
next loop, so the increment doesn't matter. Subtle! I'm happy with
whichever way Wei prefers, but it is probably worthy of a comment
>
> Jan
>
> > +
> > + } while (more_data);
> >
> > if (drop_err) {
> > netbk_tx_err(vif, first, cons + slots);
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next V2 2/3] xen-netback: avoid allocating variable size array on stack
2013-05-02 15:55 ` Ian Campbell
@ 2013-05-02 16:00 ` Wei Liu
0 siblings, 0 replies; 8+ messages in thread
From: Wei Liu @ 2013-05-02 16:00 UTC (permalink / raw)
To: Ian Campbell; +Cc: Jan Beulich, Wei Liu, xen-devel, netdev
On Thu, May 02, 2013 at 04:55:46PM +0100, Ian Campbell wrote:
> > > + more_data = txp->flags & XEN_NETTXF_more_data;
> > > +
> > > + if (!drop_err)
> > > + txp++;
> >
> > And no need for the conditional here afaict.
>
> I think it is needed, in the case where you've assigned txp =
> &dropped_tx you don't want to increment txp.
>
> Or perhaps it just gets reassigned back to &dropped_tx at the top of the
> next loop, so the increment doesn't matter. Subtle! I'm happy with
> whichever way Wei prefers, but it is probably worthy of a comment
>
Yes that's my starting point. I know that txp will always be assigned to
&dropped_tx, I just don't feel comfortable incrementing txp in that
case.
This is really a personal taste thing. :-)
Wei.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next V2 0/3] xen-netback: misc fixes
2013-05-02 10:43 [PATCH net-next V2 0/3] xen-netback: misc fixes Wei Liu
` (2 preceding siblings ...)
2013-05-02 10:43 ` [PATCH net-next V2 3/3] xen-netback: better names for thresholds Wei Liu
@ 2013-05-02 20:50 ` David Miller
3 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2013-05-02 20:50 UTC (permalink / raw)
To: wei.liu2; +Cc: xen-devel, netdev, ian.campbell, jbeulich
From: Wei Liu <wei.liu2@citrix.com>
Date: Thu, 2 May 2013 11:43:56 +0100
> This series fixes glitches spotted by Jan. Please see commit logs for details.
>
> A third patch is added to have better names for different thresholds, suggested
> by Ian Campbell.
Series applied, thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-05-02 20:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-02 10:43 [PATCH net-next V2 0/3] xen-netback: misc fixes Wei Liu
2013-05-02 10:43 ` [PATCH net-next V2 1/3] xen-netback: remove redundent parameter in netbk_count_requests Wei Liu
2013-05-02 10:43 ` [PATCH net-next V2 2/3] xen-netback: avoid allocating variable size array on stack Wei Liu
2013-05-02 12:04 ` Jan Beulich
2013-05-02 15:55 ` Ian Campbell
2013-05-02 16:00 ` Wei Liu
2013-05-02 10:43 ` [PATCH net-next V2 3/3] xen-netback: better names for thresholds Wei Liu
2013-05-02 20:50 ` [PATCH net-next V2 0/3] xen-netback: misc fixes David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).