* Re: [BUG] ipv6: all routes share same inetpeer
From: Eric Dumazet @ 2011-07-19 18:20 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110719.103724.1461298517132188126.davem@davemloft.net>
Le mardi 19 juillet 2011 à 10:37 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 19 Jul 2011 19:23:49 +0200
>
> > Maybe you can find the bug before me ?
>
> I think when we add the route we cow the metrics almost immediately.
> The daddr is, unfortunately, fully prefixed at that point.
Yes, we shall provide a second ip6_rt_copy() argument, with the
destination address.
I am testing :
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ddef80f..2a6d70a 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -72,7 +72,7 @@
#define RT6_TRACE(x...) do { ; } while (0)
#endif
-static struct rt6_info * ip6_rt_copy(struct rt6_info *ort);
+static struct rt6_info * ip6_rt_copy(struct rt6_info *ort, const struct in6_addr *dest);
static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie);
static unsigned int ip6_default_advmss(const struct dst_entry *dst);
static unsigned int ip6_default_mtu(const struct dst_entry *dst);
@@ -699,7 +699,7 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_add
* Clone the route.
*/
- rt = ip6_rt_copy(ort);
+ rt = ip6_rt_copy(ort, daddr);
if (rt) {
struct neighbour *neigh;
@@ -712,7 +712,6 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_add
ipv6_addr_copy(&rt->rt6i_gateway, daddr);
}
- ipv6_addr_copy(&rt->rt6i_dst.addr, daddr);
rt->rt6i_dst.plen = 128;
rt->rt6i_flags |= RTF_CACHE;
rt->dst.flags |= DST_HOST;
@@ -761,9 +760,9 @@ static struct rt6_info *rt6_alloc_cow(struct rt6_info *ort, const struct in6_add
static struct rt6_info *rt6_alloc_clone(struct rt6_info *ort, const struct in6_addr *daddr)
{
- struct rt6_info *rt = ip6_rt_copy(ort);
+ struct rt6_info *rt = ip6_rt_copy(ort, daddr);
+
if (rt) {
- ipv6_addr_copy(&rt->rt6i_dst.addr, daddr);
rt->rt6i_dst.plen = 128;
rt->rt6i_flags |= RTF_CACHE;
rt->dst.flags |= DST_HOST;
@@ -1584,7 +1583,7 @@ void rt6_redirect(const struct in6_addr *dest, const struct in6_addr *src,
if (neigh == dst_get_neighbour(&rt->dst))
goto out;
- nrt = ip6_rt_copy(rt);
+ nrt = ip6_rt_copy(rt, dest);
if (nrt == NULL)
goto out;
@@ -1592,7 +1591,6 @@ void rt6_redirect(const struct in6_addr *dest, const struct in6_addr *src,
if (on_link)
nrt->rt6i_flags &= ~RTF_GATEWAY;
- ipv6_addr_copy(&nrt->rt6i_dst.addr, dest);
nrt->rt6i_dst.plen = 128;
nrt->dst.flags |= DST_HOST;
@@ -1730,7 +1728,7 @@ void rt6_pmtu_discovery(const struct in6_addr *daddr, const struct in6_addr *sad
* Misc support functions
*/
-static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
+static struct rt6_info * ip6_rt_copy(struct rt6_info *ort, const struct in6_addr *dest)
{
struct net *net = dev_net(ort->rt6i_dev);
struct rt6_info *rt = ip6_dst_alloc(&net->ipv6.ip6_dst_ops,
@@ -1740,6 +1738,8 @@ static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
rt->dst.input = ort->dst.input;
rt->dst.output = ort->dst.output;
+ ipv6_addr_copy(&rt->rt6i_dst.addr, dest);
+ rt->rt6i_dst.plen = ort->rt6i_dst.plen;
dst_copy_metrics(&rt->dst, &ort->dst);
rt->dst.error = ort->dst.error;
rt->rt6i_idev = ort->rt6i_idev;
@@ -1752,7 +1752,6 @@ static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
rt->rt6i_flags = ort->rt6i_flags & ~RTF_EXPIRES;
rt->rt6i_metric = 0;
- memcpy(&rt->rt6i_dst, &ort->rt6i_dst, sizeof(struct rt6key));
#ifdef CONFIG_IPV6_SUBTREES
memcpy(&rt->rt6i_src, &ort->rt6i_src, sizeof(struct rt6key));
#endif
^ permalink raw reply related
* [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
From: Shirley Ma @ 2011-07-19 18:37 UTC (permalink / raw)
To: David Miller, mst; +Cc: netdev, jasowang
Signed-off-by: Shirley Ma <xma@us.ibm.com>
---
drivers/vhost/net.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 70ac604..83cb738 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -189,8 +189,10 @@ static void handle_tx(struct vhost_net *net)
break;
}
/* If more outstanding DMAs, queue the work */
- if (unlikely(vq->upend_idx - vq->done_idx >
- VHOST_MAX_PEND)) {
+ if (unlikely((vq->upend_idx - vq->done_idx >
+ VHOST_MAX_PEND) ||
+ (vq->upend_idx - vq->done_idx >
+ VHOST_MAX_PEND - UIO_MAXIOV))) {
tx_poll_start(net, sock);
set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
break;
^ permalink raw reply related
* Re: [PATCH 1/3] stmmac: unify MAC and PHY configuration parameters
From: David Miller @ 2011-07-19 18:57 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev, stuart.menefy
In-Reply-To: <1311064684-20246-1-git-send-email-peppe.cavallaro@st.com>
From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Tue, 19 Jul 2011 10:38:02 +0200
> Prior to this change, most PHY configuration parameters were passed
> into the STMMAC device as a separate PHY device. As well as being
> unusual, this made it difficult to make changes to the MAC/PHY
> relationship.
>
> This patch moves all the PHY parameters into the MAC configuration
> structure, mainly as a separate structure. This allows us to completly
> ignore the MDIO bus attached to a stmmac if desired, and not create
> the PHY bus. It also allows the stmmac driver to use a different PHY
> from the one it is connected to, for example a fixed PHY or bit banging
> PHY.
>
> Also derive the stmmac/PHY connection type (MII/RMII etc) from the
> mode can be passed into <platf>_configure_ethernet.
> STLinux kernel at git://git.stlinux.com/stm/linux-sh4-2.6.32.y.git
> provides several examples how to use this new infrastructure (that
> actually is easier to maintain and clearer).
>
> Signed-off-by: Stuart Menefy <stuart.menefy@st.com>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
I find these changes confusing, because I can't see where these
platform data objects are created that end up being used by
the stmmac driver.
I'm concerned about this because if you're changing these data
structures, you'll need to update also the code that creates
these platform data objects.
Finally, this patch needs to update Documentation/networking/stmmac.txt
^ permalink raw reply
* Re: [PULL net-next] vhost-net fixes for 3.1
From: Michael S. Tsirkin @ 2011-07-19 18:58 UTC (permalink / raw)
To: netdev; +Cc: jasowang, mashirle
In-Reply-To: <20110719114741.GA5827@redhat.com>
On Tue, Jul 19, 2011 at 02:47:41PM +0300, Michael S. Tsirkin wrote:
> Here's Jason's fix for a theoretical event loss
> on migration (tweaked by me to update the test
> module as well), and a refcounting fix by me
> for a bug in the new zerocopy mode.
>
> I thought hard about merging Jason's patches for 3.0
> as well, but in the end I think it's not worth
> the risk of breaking something at this stage.
>
> Please pull for 3.1, thanks a lot!
>
> The following changes since commit 81fc70d86527a1450560709500ca5f52e661da1f:
>
> net: can: remove custom hex_to_bin() (2011-07-18 11:31:43 -0700)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net-next
>
> Jason Wang (2):
> vhost: init used ring after backend was set
> vhost: set log when updating used flags or avail event
>
> Michael S. Tsirkin (1):
> vhost: fix zcopy reference counting
I have snuck in another one:
vhost: optimize interrupt enable/disable
please don't be alarmed :)
> drivers/vhost/net.c | 4 ++
> drivers/vhost/test.c | 5 +++
> drivers/vhost/vhost.c | 87 ++++++++++++++++++++++++++++++------------------
> drivers/vhost/vhost.h | 1 +
> 4 files changed, 64 insertions(+), 33 deletions(-)
^ permalink raw reply
* Re: [BUG] ipv6: all routes share same inetpeer
From: Eric Dumazet @ 2011-07-19 18:57 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <1311099638.3113.2.camel@edumazet-laptop>
Le mardi 19 juillet 2011 à 20:20 +0200, Eric Dumazet a écrit :
> Le mardi 19 juillet 2011 à 10:37 -0700, David Miller a écrit :
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Tue, 19 Jul 2011 19:23:49 +0200
> >
> > > Maybe you can find the bug before me ?
> >
> > I think when we add the route we cow the metrics almost immediately.
> > The daddr is, unfortunately, fully prefixed at that point.
>
> Yes, we shall provide a second ip6_rt_copy() argument, with the
> destination address.
>
Hmm, or maybe just change the dst_copy_metrics(&rt->dst, &ort->dst);
call done from ip6_rt_copy(), to avoid doing the COW if not really
needed ?
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ddef80f..5403cea 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1740,7 +1740,7 @@ static struct rt6_info * ip6_rt_copy(struct rt6_info *ort)
rt->dst.input = ort->dst.input;
rt->dst.output = ort->dst.output;
- dst_copy_metrics(&rt->dst, &ort->dst);
+ rt->dst._metrics = ort->dst._metrics;
rt->dst.error = ort->dst.error;
rt->rt6i_idev = ort->rt6i_idev;
if (rt->rt6i_idev)
^ permalink raw reply related
* Re: [BUG] ipv6: all routes share same inetpeer
From: David Miller @ 2011-07-19 18:59 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1311101870.3113.6.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 19 Jul 2011 20:57:50 +0200
> Le mardi 19 juillet 2011 à 20:20 +0200, Eric Dumazet a écrit :
>> Le mardi 19 juillet 2011 à 10:37 -0700, David Miller a écrit :
>> > From: Eric Dumazet <eric.dumazet@gmail.com>
>> > Date: Tue, 19 Jul 2011 19:23:49 +0200
>> >
>> > > Maybe you can find the bug before me ?
>> >
>> > I think when we add the route we cow the metrics almost immediately.
>> > The daddr is, unfortunately, fully prefixed at that point.
>>
>> Yes, we shall provide a second ip6_rt_copy() argument, with the
>> destination address.
>>
>
> Hmm, or maybe just change the dst_copy_metrics(&rt->dst, &ort->dst);
> call done from ip6_rt_copy(), to avoid doing the COW if not really
> needed ?
This is ok if it handles the case where ort's metrics point to
writable inetpeer memory.
^ permalink raw reply
* Re: [PULL net-next] vhost-net fixes for 3.1
From: David Miller @ 2011-07-19 19:00 UTC (permalink / raw)
To: mst; +Cc: netdev, jasowang, mashirle
In-Reply-To: <20110719185813.GA8632@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 19 Jul 2011 21:58:13 +0300
> I have snuck in another one:
> vhost: optimize interrupt enable/disable
>
> please don't be alarmed :)
I already pulled so send another pull request if you want me
to get this.
^ permalink raw reply
* Re: [PULL net-next] vhost-net fixes for 3.1
From: Michael S. Tsirkin @ 2011-07-19 19:05 UTC (permalink / raw)
To: David Miller; +Cc: netdev, jasowang, mashirle
In-Reply-To: <20110719.120007.1487641546240582444.davem@davemloft.net>
On Tue, Jul 19, 2011 at 12:00:07PM -0700, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Tue, 19 Jul 2011 21:58:13 +0300
>
> > I have snuck in another one:
> > vhost: optimize interrupt enable/disable
> >
> > please don't be alarmed :)
>
> I already pulled so send another pull request if you want me
> to get this.
You got the new one actually, so nothing needs to be done,
I think.
Thanks!
--
MST
^ permalink raw reply
* Re: [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
From: Michael S. Tsirkin @ 2011-07-19 19:09 UTC (permalink / raw)
To: Shirley Ma; +Cc: David Miller, netdev, jasowang
In-Reply-To: <1311100678.8573.16.camel@localhost.localdomain>
On Tue, Jul 19, 2011 at 11:37:58AM -0700, Shirley Ma wrote:
> Signed-off-by: Shirley Ma <xma@us.ibm.com>
> ---
>
> drivers/vhost/net.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 70ac604..83cb738 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -189,8 +189,10 @@ static void handle_tx(struct vhost_net *net)
> break;
> }
> /* If more outstanding DMAs, queue the work */
> - if (unlikely(vq->upend_idx - vq->done_idx >
> - VHOST_MAX_PEND)) {
> + if (unlikely((vq->upend_idx - vq->done_idx >
> + VHOST_MAX_PEND) ||
> + (vq->upend_idx - vq->done_idx >
> + VHOST_MAX_PEND - UIO_MAXIOV))) {
Could you please explain why this makes sense please?
VHOST_MAX_PEND is 128 UIO_MAXIOV is 1024 so
the result is negative?
I thought upend_idx - done_idx is exactly the number
of buffers, so once we get too many we stop until
one gets freed?
> tx_poll_start(net, sock);
> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> break;
>
^ permalink raw reply
* Re: [PATCH] vhost: clean up outstanding buffers before setting vring
From: Michael S. Tsirkin @ 2011-07-19 19:49 UTC (permalink / raw)
To: Shirley Ma; +Cc: David Miller, netdev, jasowang
In-Reply-To: <1311098546.8573.13.camel@localhost.localdomain>
On Tue, Jul 19, 2011 at 11:02:26AM -0700, Shirley Ma wrote:
> The outstanding DMA buffers need to be clean up before setting vring in
> vhost. Otherwise the vring would be out of sync.
>
> Signed-off-by: Shirley Ma<xma@us.ibm.com>
I suspect what is missing is calling
vhost_zerocopy_signal_used then?
If yes we probably should do it after
changing the backend, not on vring set.
> ---
>
> drivers/vhost/vhost.c | 11 +++++++++--
> 1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index c14c42b..d6315b4 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -445,8 +445,10 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> vhost_poll_flush(&dev->vqs[i].poll);
> }
> /* Wait for all lower device DMAs done. */
> - if (dev->vqs[i].ubufs)
> + if (dev->vqs[i].ubufs) {
> vhost_ubuf_put_and_wait(dev->vqs[i].ubufs);
> + kfree(dev->vqs[i].ubufs);
> + }
>
> /* Signal guest as appropriate. */
> vhost_zerocopy_signal_used(&dev->vqs[i]);
> @@ -651,6 +653,12 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> vq = d->vqs + idx;
>
> mutex_lock(&vq->mutex);
> + /* Wait for all lower device DMAs done. */
> + if (vq->ubufs)
> + vhost_ubuf_put_and_wait(vq->ubufs);
Could you elaborate on the problem you observe please?
At least in theory, existing code flushes outstanding
requests when backend is changed.
And since vring set verifies no backend is active,
we should be fine?
> +
> + /* Signal guest as appropriate. */
> + vhost_zerocopy_signal_used(vq);
>
> switch (ioctl) {
> case VHOST_SET_VRING_NUM:
> @@ -1592,7 +1600,6 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs)
> {
> kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
> wait_event(ubufs->wait, !atomic_read(&ubufs->kref.refcount));
> - kfree(ubufs);
Won't this leak memory when ubufs are switched in vhost_net_set_backend?
> }
>
> void vhost_zerocopy_callback(void *arg)
>
>
>
^ permalink raw reply
* [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods
From: Neil Horman @ 2011-07-19 19:52 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, Eric Dumazet, Alexey Dobriyan, David S. Miller
This oops was reported recently when running a pktgen script that called for a
transmitted skb to be cloned and sent 100 times using the clone_skb command to a
bridge device with several attached interface:
BUG: unable to handle kernel paging request at ffff880136994528
RIP: 0010:[<ffff880136994528>] [<ffff880136994528>] 0xffff880136994528
RSP: 0018:ffff8801384e5b78 EFLAGS: 00010286
RAX: ffff880136994528 RBX: ffff880137e52800 RCX: 0000000000000000
RDX: ffffffffa0529ba0 RSI: ffff880137e52800 RDI: ffff8801369944b0
RBP: ffff8801384e5ba0 R08: ffff8801379cb49c R09: ffff8801384e5c78
R10: 0000000000000000 R11: 0000000000000400 R12: ffff880137e52ec0
R13: ffff8801369944b0 R14: ffff880136ed9480 R15: ffff880137e52800
FS: 0000000000000000(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: ffff880136994528 CR3: 00000001395f2000 CR4: 00000000000026e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kpktgend_0 (pid: 2601, threadinfo ffff8801384e4000, task
ffff880137fe5560)
Stack:
ffffffffa0527425 0000000000000000 ffff8801369944b0 ffff880137e52800
<0> ffff880136ed9480 ffff8801384e5bf0 ffffffff8141047a ffffffffa0529ba0
<0> ffff880134813e40 0000000080000000 ffff8801379cb400 ffff880137e52800
Call Trace:
[<ffffffffa0527425>] ? tun_net_xmit+0x135/0x200 [tun]
[<ffffffff8141047a>] dev_hard_start_xmit+0x20a/0x370
[<ffffffff814288aa>] sch_direct_xmit+0x15a/0x1c0
[<ffffffff81413ab8>] dev_queue_xmit+0x378/0x4a0
[<ffffffffa0488400>] ? br_dev_queue_push_xmit+0x0/0xa0 [bridge]
[<ffffffffa048846c>] br_dev_queue_push_xmit+0x6c/0xa0 [bridge]
[<ffffffffa04884f8>] br_forward_finish+0x58/0x60 [bridge]
[<ffffffffa0488690>] __br_deliver+0x60/0x70 [bridge]
[<ffffffffa04883aa>] br_flood+0xca/0xe0 [bridge]
[<ffffffffa0488630>] ? __br_deliver+0x0/0x70 [bridge]
[<ffffffffa04883f7>] br_flood_deliver+0x17/0x20 [bridge]
[<ffffffffa048748b>] br_dev_xmit+0xfb/0x100 [bridge]
[<ffffffffa053790e>] pktgen_thread_worker+0x83e/0x1bc8 [pktgen]
[<ffffffff81059d12>] ? finish_task_switch+0x42/0xd0
[<ffffffffa0487390>] ? br_dev_xmit+0x0/0x100 [bridge]
[<ffffffff81091ca0>] ? autoremove_wake_function+0x0/0x40
[<ffffffff81091ca0>] ? autoremove_wake_function+0x0/0x40
[<ffffffffa05370d0>] ? pktgen_thread_worker+0x0/0x1bc8 [pktgen]
[<ffffffff81091936>] kthread+0x96/0xa0
[<ffffffff810141ca>] child_rip+0xa/0x20
[<ffffffff810918a0>] ? kthread+0x0/0xa0
[<ffffffff810141c0>] ? child_rip+0x0/0x20
Turns out the pktgen driver doesn't actually clone skbs, but rather shares them,
increasing the reference count of the skb for each send operation. This works
for most drivers because most drivers don't store or care about any state in the
skb itself, but several do. For instance, the above tun/tap driver and other
soft drivers (vlans, bonding/bridging), all requeue frames to a physical device,
meaning the skb next and prev pointers will be set. Other drivers also care
about skb state. The virtio_net driver for instance uses the skb->cb space to
store a vnet header and several converged adapters adjust the data pointer of an
skb to prepend a device control header to the skb. Drivers expect skbs
submitted for i/o to be in their control and unshared with other users, an
assumption which pktgen is violating, the result being multiple skb users
corrupting one antohers state and producing oopses like the one above. The
solution is to make pktgen clone the skb for each transmit so as to ensure the
drivers assumptions about private exclusive access to the skb is maintained.
Tested successfully by myself
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Jiri Pirko <jpirko@redhat.com>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: David S. Miller <davem@davemloft.net>
---
net/core/pktgen.c | 21 +++++++++++++--------
1 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f76079c..c8e0e08 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -3297,6 +3297,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
netdev_tx_t (*xmit)(struct sk_buff *, struct net_device *)
= odev->netdev_ops->ndo_start_xmit;
struct netdev_queue *txq;
+ struct sk_buff *tx_skb = NULL;
u16 queue_map;
int ret;
@@ -3316,9 +3317,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
/* If no skb or clone count exhausted then get new one */
if (!pkt_dev->skb || (pkt_dev->last_ok &&
- ++pkt_dev->clone_count >= pkt_dev->clone_skb)) {
- /* build a new pkt */
- kfree_skb(pkt_dev->skb);
+ ++pkt_dev->clone_count > pkt_dev->clone_skb)) {
pkt_dev->skb = fill_packet(odev, pkt_dev);
if (pkt_dev->skb == NULL) {
@@ -3332,10 +3331,13 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
pkt_dev->clone_count = 0; /* reset counter */
}
+ tx_skb = (pkt_dev->clone_count == pkt_dev->clone_skb) ?
+ pkt_dev->skb : skb_clone(pkt_dev->skb, GFP_KERNEL);
+
if (pkt_dev->delay && pkt_dev->last_ok)
spin(pkt_dev, pkt_dev->next_tx);
- queue_map = skb_get_queue_mapping(pkt_dev->skb);
+ queue_map = skb_get_queue_mapping(tx_skb);
txq = netdev_get_tx_queue(odev, queue_map);
__netif_tx_lock_bh(txq);
@@ -3345,8 +3347,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
pkt_dev->last_ok = 0;
goto unlock;
}
- atomic_inc(&(pkt_dev->skb->users));
- ret = (*xmit)(pkt_dev->skb, odev);
+
+ ret = (*xmit)(tx_skb, odev);
switch (ret) {
case NETDEV_TX_OK:
@@ -3369,8 +3371,11 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
/* fallthru */
case NETDEV_TX_LOCKED:
case NETDEV_TX_BUSY:
- /* Retry it next time */
- atomic_dec(&(pkt_dev->skb->users));
+ /*
+ * Only free it if its not the last in the clone series
+ */
+ if (tx_skb != pkt_dev->skb)
+ kfree_skb(tx_skb);
pkt_dev->last_ok = 0;
}
unlock:
--
1.7.6
^ permalink raw reply related
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods
From: Eric Dumazet @ 2011-07-19 20:02 UTC (permalink / raw)
To: Neil Horman; +Cc: netdev, Alexey Dobriyan, David S. Miller
In-Reply-To: <1311105179-26408-1-git-send-email-nhorman@tuxdriver.com>
Le mardi 19 juillet 2011 à 15:52 -0400, Neil Horman a écrit :
> This oops was reported recently when running a pktgen script that called for a
> transmitted skb to be cloned and sent 100 times using the clone_skb command to a
> bridge device with several attached interface:
>
...
> Turns out the pktgen driver doesn't actually clone skbs, but rather shares them,
> increasing the reference count of the skb for each send operation. This works
> for most drivers because most drivers don't store or care about any state in the
> skb itself, but several do. For instance, the above tun/tap driver and other
> soft drivers (vlans, bonding/bridging), all requeue frames to a physical device,
> meaning the skb next and prev pointers will be set. Other drivers also care
> about skb state. The virtio_net driver for instance uses the skb->cb space to
> store a vnet header and several converged adapters adjust the data pointer of an
> skb to prepend a device control header to the skb. Drivers expect skbs
> submitted for i/o to be in their control and unshared with other users, an
> assumption which pktgen is violating, the result being multiple skb users
> corrupting one antohers state and producing oopses like the one above. The
> solution is to make pktgen clone the skb for each transmit so as to ensure the
> drivers assumptions about private exclusive access to the skb is maintained.
>
> Tested successfully by myself
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Jiri Pirko <jpirko@redhat.com>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Alexey Dobriyan <adobriyan@gmail.com>
> CC: David S. Miller <davem@davemloft.net>
> ---
This will kill pktgen performance ?
pktgen is only for sysadmins, and very skilled ones :)
BTW you forgot to CC pktgen author, Robert Olsson
^ permalink raw reply
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods
From: Joe Perches @ 2011-07-19 20:17 UTC (permalink / raw)
To: Eric Dumazet, Robert Olsson
Cc: Neil Horman, netdev, Alexey Dobriyan, David S. Miller
In-Reply-To: <1311105738.3113.11.camel@edumazet-laptop>
On Tue, 2011-07-19 at 22:02 +0200, Eric Dumazet wrote:
> Le mardi 19 juillet 2011 à 15:52 -0400, Neil Horman a écrit :
> > This oops was reported recently when running a pktgen script that called for a
> > transmitted skb to be cloned and sent 100 times using the clone_skb command to a
> > bridge device with several attached interface:
> BTW you forgot to CC pktgen author, Robert Olsson
Robert, you're not getting cc's on pktgen when
people use get_maintainer.
Are you still interested in getting cc'd on patches
to pktgen?
If so, perhaps you want to add a MAINTAINERS entry
for it.
Maybe something like:
diff --git a/MAINTAINERS b/MAINTAINERS
index 378fccf..5a6c16a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4999,6 +4999,11 @@ S: Maintained
F: drivers/block/pktcdvd.c
F: include/linux/pktcdvd.h
+PKTGEN
+M: Robert Olsson <robert.olsson@its.uu.se>
+S: Odd Fixes
+F: net/core/pktgen.c
+
PKUNITY SOC DRIVERS
M: Guan Xuetao <gxt@mprc.pku.edu.cn>
W: http://mprc.pku.edu.cn/~guanxuetao/linux
^ permalink raw reply related
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods
From: Jiri Pirko @ 2011-07-19 20:29 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Neil Horman, netdev, Alexey Dobriyan, David S. Miller
In-Reply-To: <1311105738.3113.11.camel@edumazet-laptop>
Tue, Jul 19, 2011 at 10:02:18PM CEST, eric.dumazet@gmail.com wrote:
>Le mardi 19 juillet 2011 à 15:52 -0400, Neil Horman a écrit :
>> This oops was reported recently when running a pktgen script that called for a
>> transmitted skb to be cloned and sent 100 times using the clone_skb command to a
>> bridge device with several attached interface:
>>
>...
>
>> Turns out the pktgen driver doesn't actually clone skbs, but rather shares them,
>> increasing the reference count of the skb for each send operation. This works
>> for most drivers because most drivers don't store or care about any state in the
>> skb itself, but several do. For instance, the above tun/tap driver and other
>> soft drivers (vlans, bonding/bridging), all requeue frames to a physical device,
>> meaning the skb next and prev pointers will be set. Other drivers also care
>> about skb state. The virtio_net driver for instance uses the skb->cb space to
>> store a vnet header and several converged adapters adjust the data pointer of an
>> skb to prepend a device control header to the skb. Drivers expect skbs
>> submitted for i/o to be in their control and unshared with other users, an
>> assumption which pktgen is violating, the result being multiple skb users
>> corrupting one antohers state and producing oopses like the one above. The
>> solution is to make pktgen clone the skb for each transmit so as to ensure the
>> drivers assumptions about private exclusive access to the skb is maintained.
>>
>> Tested successfully by myself
>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>> Reported-by: Jiri Pirko <jpirko@redhat.com>
>> CC: Eric Dumazet <eric.dumazet@gmail.com>
>> CC: Alexey Dobriyan <adobriyan@gmail.com>
>> CC: David S. Miller <davem@davemloft.net>
>> ---
>
>This will kill pktgen performance ?
>
>pktgen is only for sysadmins, and very skilled ones :)
You are right, but it may not cause panic, right? In case this patch
would cause significant performance regression, how about to just forbid
pktgen to run on soft-netdevs ?
Jirka
>
>BTW you forgot to CC pktgen author, Robert Olsson
>
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods
From: Eric Dumazet @ 2011-07-19 20:41 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Neil Horman, netdev, Alexey Dobriyan, David S. Miller
In-Reply-To: <20110719202922.GA2352@minipsycho>
Le mardi 19 juillet 2011 à 22:29 +0200, Jiri Pirko a écrit :
> You are right, but it may not cause panic, right? In case this patch
> would cause significant performance regression, how about to just forbid
> pktgen to run on soft-netdevs ?
>
Please do
Note : a sysadmin has other ways to make a machine panic or reboot or
halt...
^ permalink raw reply
* [PATCH net-next-2.6] ipv6: make fragment identifications less predictable
From: Eric Dumazet @ 2011-07-19 20:47 UTC (permalink / raw)
To: Fernando Gont, David Miller; +Cc: security, Eugene Teo, netdev, Matt Mackall
In-Reply-To: <1311089463.2375.42.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
IPv6 fragment identification generation is way beyond what we use for
IPv4 : It uses a single generator. Its not scalable and allows DOS
attacks.
Now inetpeer is IPv6 aware, we can use it to provide a more secure and
scalable frag ident generator (per destination, instead of system wide)
This patch :
1) defines a new secure_ipv6_id() helper
2) extends inet_getid() to provide 32bit results
3) extends ipv6_select_ident() with a new dest parameter
Reported-by: Fernando Gont <fernando@gont.com.ar>
CC: Matt Mackall <mpm@selenic.com>
CC: Eugene Teo <eugeneteo@kernel.sg>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
drivers/char/random.c | 15 +++++++++++++++
include/linux/random.h | 1 +
include/net/inetpeer.h | 13 ++++++++++---
include/net/ipv6.h | 12 +-----------
net/ipv4/inetpeer.c | 7 +++++--
net/ipv6/ip6_output.c | 36 +++++++++++++++++++++++++++++++-----
net/ipv6/udp.c | 2 +-
7 files changed, 64 insertions(+), 22 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index d4ddeba..7292819 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1523,6 +1523,21 @@ __u32 secure_ip_id(__be32 daddr)
return half_md4_transform(hash, keyptr->secret);
}
+__u32 secure_ipv6_id(const __be32 daddr[4])
+{
+ const struct keydata *keyptr;
+ __u32 hash[4];
+
+ keyptr = get_keyptr();
+
+ hash[0] = (__force __u32)daddr[0];
+ hash[1] = (__force __u32)daddr[1];
+ hash[2] = (__force __u32)daddr[2];
+ hash[3] = (__force __u32)daddr[3];
+
+ return half_md4_transform(hash, keyptr->secret);
+}
+
#ifdef CONFIG_INET
__u32 secure_tcp_sequence_number(__be32 saddr, __be32 daddr,
diff --git a/include/linux/random.h b/include/linux/random.h
index fb7ab9d..ce29a04 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -58,6 +58,7 @@ extern void get_random_bytes(void *buf, int nbytes);
void generate_random_uuid(unsigned char uuid_out[16]);
extern __u32 secure_ip_id(__be32 daddr);
+extern __u32 secure_ipv6_id(const __be32 daddr[4]);
extern u32 secure_ipv4_port_ephemeral(__be32 saddr, __be32 daddr, __be16 dport);
extern u32 secure_ipv6_port_ephemeral(const __be32 *saddr, const __be32 *daddr,
__be16 dport);
diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h
index 39d1230..4233e6f 100644
--- a/include/net/inetpeer.h
+++ b/include/net/inetpeer.h
@@ -71,7 +71,7 @@ static inline bool inet_metrics_new(const struct inet_peer *p)
}
/* can be called with or without local BH being disabled */
-struct inet_peer *inet_getpeer(struct inetpeer_addr *daddr, int create);
+struct inet_peer *inet_getpeer(const struct inetpeer_addr *daddr, int create);
static inline struct inet_peer *inet_getpeer_v4(__be32 v4daddr, int create)
{
@@ -106,11 +106,18 @@ static inline void inet_peer_refcheck(const struct inet_peer *p)
/* can be called with or without local BH being disabled */
-static inline __u16 inet_getid(struct inet_peer *p, int more)
+static inline int inet_getid(struct inet_peer *p, int more)
{
+ int old, new;
more++;
inet_peer_refcheck(p);
- return atomic_add_return(more, &p->ip_id_count) - more;
+ do {
+ old = atomic_read(&p->ip_id_count);
+ new = old + more;
+ if (!new)
+ new = 1;
+ } while (atomic_cmpxchg(&p->ip_id_count, old, new) != old);
+ return new;
}
#endif /* _NET_INETPEER_H */
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index c033ed0..3b5ac1f 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -463,17 +463,7 @@ static inline int ipv6_addr_diff(const struct in6_addr *a1, const struct in6_add
return __ipv6_addr_diff(a1, a2, sizeof(struct in6_addr));
}
-static __inline__ void ipv6_select_ident(struct frag_hdr *fhdr)
-{
- static u32 ipv6_fragmentation_id = 1;
- static DEFINE_SPINLOCK(ip6_id_lock);
-
- spin_lock_bh(&ip6_id_lock);
- fhdr->identification = htonl(ipv6_fragmentation_id);
- if (++ipv6_fragmentation_id == 0)
- ipv6_fragmentation_id = 1;
- spin_unlock_bh(&ip6_id_lock);
-}
+extern void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt);
/*
* Prototypes exported by ipv6
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index 90c5f0d..e382138 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -391,7 +391,7 @@ static int inet_peer_gc(struct inet_peer_base *base,
return cnt;
}
-struct inet_peer *inet_getpeer(struct inetpeer_addr *daddr, int create)
+struct inet_peer *inet_getpeer(const struct inetpeer_addr *daddr, int create)
{
struct inet_peer __rcu **stack[PEER_MAXDEPTH], ***stackptr;
struct inet_peer_base *base = family_to_base(daddr->family);
@@ -436,7 +436,10 @@ relookup:
p->daddr = *daddr;
atomic_set(&p->refcnt, 1);
atomic_set(&p->rid, 0);
- atomic_set(&p->ip_id_count, secure_ip_id(daddr->addr.a4));
+ atomic_set(&p->ip_id_count,
+ (daddr->family == AF_INET) ?
+ secure_ip_id(daddr->addr.a4) :
+ secure_ipv6_id(daddr->addr.a6));
p->tcp_ts_stamp = 0;
p->metrics[RTAX_LOCK-1] = INETPEER_METRICS_NEW;
p->rate_tokens = 0;
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 8db0e48..32e5339 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -596,6 +596,31 @@ int ip6_find_1stfragopt(struct sk_buff *skb, u8 **nexthdr)
return offset;
}
+void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt)
+{
+ static atomic_t ipv6_fragmentation_id;
+ int old, new;
+
+ if (rt) {
+ struct inet_peer *peer;
+
+ if (!rt->rt6i_peer)
+ rt6_bind_peer(rt, 1);
+ peer = rt->rt6i_peer;
+ if (peer) {
+ fhdr->identification = htonl(inet_getid(peer, 0));
+ return;
+ }
+ }
+ do {
+ old = atomic_read(&ipv6_fragmentation_id);
+ new = old + 1;
+ if (!new)
+ new = 1;
+ } while (atomic_cmpxchg(&ipv6_fragmentation_id, old, new) != old);
+ fhdr->identification = htonl(new);
+}
+
int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
{
struct sk_buff *frag;
@@ -680,7 +705,7 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
skb_reset_network_header(skb);
memcpy(skb_network_header(skb), tmp_hdr, hlen);
- ipv6_select_ident(fh);
+ ipv6_select_ident(fh, rt);
fh->nexthdr = nexthdr;
fh->reserved = 0;
fh->frag_off = htons(IP6_MF);
@@ -826,7 +851,7 @@ slow_path:
fh->nexthdr = nexthdr;
fh->reserved = 0;
if (!frag_id) {
- ipv6_select_ident(fh);
+ ipv6_select_ident(fh, rt);
frag_id = fh->identification;
} else
fh->identification = frag_id;
@@ -1076,7 +1101,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
int getfrag(void *from, char *to, int offset, int len,
int odd, struct sk_buff *skb),
void *from, int length, int hh_len, int fragheaderlen,
- int transhdrlen, int mtu,unsigned int flags)
+ int transhdrlen, int mtu,unsigned int flags,
+ struct rt6_info *rt)
{
struct sk_buff *skb;
@@ -1120,7 +1146,7 @@ static inline int ip6_ufo_append_data(struct sock *sk,
skb_shinfo(skb)->gso_size = (mtu - fragheaderlen -
sizeof(struct frag_hdr)) & ~7;
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
- ipv6_select_ident(&fhdr);
+ ipv6_select_ident(&fhdr, rt);
skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
__skb_queue_tail(&sk->sk_write_queue, skb);
@@ -1286,7 +1312,7 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to,
err = ip6_ufo_append_data(sk, getfrag, from, length,
hh_len, fragheaderlen,
- transhdrlen, mtu, flags);
+ transhdrlen, mtu, flags, rt);
if (err)
goto error;
return 0;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 328985c..29213b5 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1359,7 +1359,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, u32 features)
fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
fptr->nexthdr = nexthdr;
fptr->reserved = 0;
- ipv6_select_ident(fptr);
+ ipv6_select_ident(fptr, (struct rt6_info *)skb_dst(skb));
/* Fragment the skb. ipv6 header and the remaining fields of the
* fragment header are updated in ipv6_gso_segment()
^ permalink raw reply related
* Re: [PATCH] vhost: clean up outstanding buffers before setting vring
From: Shirley Ma @ 2011-07-19 20:50 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: David Miller, netdev, jasowang
In-Reply-To: <20110719194956.GC8667@redhat.com>
On Tue, 2011-07-19 at 22:49 +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2011 at 11:02:26AM -0700, Shirley Ma wrote:
> > The outstanding DMA buffers need to be clean up before setting vring
> in
> > vhost. Otherwise the vring would be out of sync.
> >
> > Signed-off-by: Shirley Ma<xma@us.ibm.com>
>
> I suspect what is missing is calling
> vhost_zerocopy_signal_used then?
>
> If yes we probably should do it after
> changing the backend, not on vring set.
I think vhost_zerocopy_signal_used might not be sufficient. But we can
test it out by remove/reloading the guest virtio_net module.
> > ---
> >
> > drivers/vhost/vhost.c | 11 +++++++++--
> > 1 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index c14c42b..d6315b4 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -445,8 +445,10 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > vhost_poll_flush(&dev->vqs[i].poll);
> > }
> > /* Wait for all lower device DMAs done. */
> > - if (dev->vqs[i].ubufs)
> > + if (dev->vqs[i].ubufs) {
> > vhost_ubuf_put_and_wait(dev->vqs[i].ubufs);
> > + kfree(dev->vqs[i].ubufs);
> > + }
> >
> > /* Signal guest as appropriate. */
> > vhost_zerocopy_signal_used(&dev->vqs[i]);
> > @@ -651,6 +653,12 @@ static long vhost_set_vring(struct vhost_dev
> *d, int ioctl, void __user *argp)
> > vq = d->vqs + idx;
> >
> > mutex_lock(&vq->mutex);
> > + /* Wait for all lower device DMAs done. */
> > + if (vq->ubufs)
> > + vhost_ubuf_put_and_wait(vq->ubufs);
>
> Could you elaborate on the problem you observe please?
> At least in theory, existing code flushes outstanding
> requests when backend is changed.
> And since vring set verifies no backend is active,
> we should be fine?
The problem encounters when guest rmmod virtio_net module, then reload
the module, and configure the interface, it complains about some ring id
is not a head. With this patch, the problem is solved.
>
> > +
> > + /* Signal guest as appropriate. */
> > + vhost_zerocopy_signal_used(vq);
> >
> > switch (ioctl) {
> > case VHOST_SET_VRING_NUM:
> > @@ -1592,7 +1600,6 @@ void vhost_ubuf_put_and_wait(struct
> vhost_ubuf_ref *ubufs)
> > {
> > kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
> > wait_event(ubufs->wait, !atomic_read(&ubufs->kref.refcount));
> > - kfree(ubufs);
>
> Won't this leak memory when ubufs are switched in
> vhost_net_set_backend?
Right, I forgot to check net.c, whenever it calls
vhot_ubuf_put_and_wait, it should call kfree(ubufs).
^ permalink raw reply
* Re: [PATCH net-next]vhost: fix condition check for # of outstanding dma buffers
From: Shirley Ma @ 2011-07-19 20:56 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: David Miller, netdev, jasowang
In-Reply-To: <20110719190945.GB8667@redhat.com>
On Tue, 2011-07-19 at 22:09 +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2011 at 11:37:58AM -0700, Shirley Ma wrote:
> > Signed-off-by: Shirley Ma <xma@us.ibm.com>
> > ---
> >
> > drivers/vhost/net.c | 6 ++++--
> > 1 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 70ac604..83cb738 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -189,8 +189,10 @@ static void handle_tx(struct vhost_net *net)
> > break;
> > }
> > /* If more outstanding DMAs, queue the work */
> > - if (unlikely(vq->upend_idx - vq->done_idx >
> > - VHOST_MAX_PEND)) {
> > + if (unlikely((vq->upend_idx - vq->done_idx >
> > + VHOST_MAX_PEND) ||
> > + (vq->upend_idx - vq->done_idx >
> > + VHOST_MAX_PEND -
> UIO_MAXIOV))) {
>
> Could you please explain why this makes sense please?
> VHOST_MAX_PEND is 128 UIO_MAXIOV is 1024 so
> the result is negative?
I thought it is equal to:
if (vq->upend_idx > vq->done_idx)
check vq->upend_idx - vq->done_idx > VHOST_MAX_PEND
if (vq->upend_idx < vq->done_idx)
check vq->upend_idx + UIO_MAXIOV - vq->done_idx > VHOST_MAX_PEND
> I thought upend_idx - done_idx is exactly the number
> of buffers, so once we get too many we stop until
> one gets freed?
They are index, so in vhost zerocopy callback, we can get the idx right
away.
>
> > tx_poll_start(net, sock);
> > set_bit(SOCK_ASYNC_NOSPACE,
> &sock->flags);
> > break;
> >
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv6: make fragment identifications less predictable
From: Matt Mackall @ 2011-07-19 20:56 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Fernando Gont, David Miller, security, Eugene Teo, netdev
In-Reply-To: <1311108423.3113.24.camel@edumazet-laptop>
On Tue, 2011-07-19 at 22:47 +0200, Eric Dumazet wrote:
> IPv6 fragment identification generation is way beyond what we use for
> IPv4 : It uses a single generator. Its not scalable and allows DOS
> attacks.
>
> Now inetpeer is IPv6 aware, we can use it to provide a more secure and
> scalable frag ident generator (per destination, instead of system wide)
This code really needs to get moved out of random.c and into net/. Other
than that, looks fine to me.
> This patch :
> 1) defines a new secure_ipv6_id() helper
> 2) extends inet_getid() to provide 32bit results
> 3) extends ipv6_select_ident() with a new dest parameter
>
> Reported-by: Fernando Gont <fernando@gont.com.ar>
> CC: Matt Mackall <mpm@selenic.com>
> CC: Eugene Teo <eugeneteo@kernel.sg>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> drivers/char/random.c | 15 +++++++++++++++
> include/linux/random.h | 1 +
> include/net/inetpeer.h | 13 ++++++++++---
> include/net/ipv6.h | 12 +-----------
> net/ipv4/inetpeer.c | 7 +++++--
> net/ipv6/ip6_output.c | 36 +++++++++++++++++++++++++++++++-----
> net/ipv6/udp.c | 2 +-
> 7 files changed, 64 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index d4ddeba..7292819 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1523,6 +1523,21 @@ __u32 secure_ip_id(__be32 daddr)
> return half_md4_transform(hash, keyptr->secret);
> }
>
> +__u32 secure_ipv6_id(const __be32 daddr[4])
> +{
> + const struct keydata *keyptr;
> + __u32 hash[4];
> +
> + keyptr = get_keyptr();
> +
> + hash[0] = (__force __u32)daddr[0];
> + hash[1] = (__force __u32)daddr[1];
> + hash[2] = (__force __u32)daddr[2];
> + hash[3] = (__force __u32)daddr[3];
> +
> + return half_md4_transform(hash, keyptr->secret);
> +}
> +
> #ifdef CONFIG_INET
>
> __u32 secure_tcp_sequence_number(__be32 saddr, __be32 daddr,
> diff --git a/include/linux/random.h b/include/linux/random.h
> index fb7ab9d..ce29a04 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -58,6 +58,7 @@ extern void get_random_bytes(void *buf, int nbytes);
> void generate_random_uuid(unsigned char uuid_out[16]);
>
> extern __u32 secure_ip_id(__be32 daddr);
> +extern __u32 secure_ipv6_id(const __be32 daddr[4]);
> extern u32 secure_ipv4_port_ephemeral(__be32 saddr, __be32 daddr, __be16 dport);
> extern u32 secure_ipv6_port_ephemeral(const __be32 *saddr, const __be32 *daddr,
> __be16 dport);
> diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h
> index 39d1230..4233e6f 100644
> --- a/include/net/inetpeer.h
> +++ b/include/net/inetpeer.h
> @@ -71,7 +71,7 @@ static inline bool inet_metrics_new(const struct inet_peer *p)
> }
>
> /* can be called with or without local BH being disabled */
> -struct inet_peer *inet_getpeer(struct inetpeer_addr *daddr, int create);
> +struct inet_peer *inet_getpeer(const struct inetpeer_addr *daddr, int create);
>
> static inline struct inet_peer *inet_getpeer_v4(__be32 v4daddr, int create)
> {
> @@ -106,11 +106,18 @@ static inline void inet_peer_refcheck(const struct inet_peer *p)
>
>
> /* can be called with or without local BH being disabled */
> -static inline __u16 inet_getid(struct inet_peer *p, int more)
> +static inline int inet_getid(struct inet_peer *p, int more)
> {
> + int old, new;
> more++;
> inet_peer_refcheck(p);
> - return atomic_add_return(more, &p->ip_id_count) - more;
> + do {
> + old = atomic_read(&p->ip_id_count);
> + new = old + more;
> + if (!new)
> + new = 1;
> + } while (atomic_cmpxchg(&p->ip_id_count, old, new) != old);
> + return new;
> }
>
> #endif /* _NET_INETPEER_H */
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index c033ed0..3b5ac1f 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -463,17 +463,7 @@ static inline int ipv6_addr_diff(const struct in6_addr *a1, const struct in6_add
> return __ipv6_addr_diff(a1, a2, sizeof(struct in6_addr));
> }
>
> -static __inline__ void ipv6_select_ident(struct frag_hdr *fhdr)
> -{
> - static u32 ipv6_fragmentation_id = 1;
> - static DEFINE_SPINLOCK(ip6_id_lock);
> -
> - spin_lock_bh(&ip6_id_lock);
> - fhdr->identification = htonl(ipv6_fragmentation_id);
> - if (++ipv6_fragmentation_id == 0)
> - ipv6_fragmentation_id = 1;
> - spin_unlock_bh(&ip6_id_lock);
> -}
> +extern void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt);
>
> /*
> * Prototypes exported by ipv6
> diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
> index 90c5f0d..e382138 100644
> --- a/net/ipv4/inetpeer.c
> +++ b/net/ipv4/inetpeer.c
> @@ -391,7 +391,7 @@ static int inet_peer_gc(struct inet_peer_base *base,
> return cnt;
> }
>
> -struct inet_peer *inet_getpeer(struct inetpeer_addr *daddr, int create)
> +struct inet_peer *inet_getpeer(const struct inetpeer_addr *daddr, int create)
> {
> struct inet_peer __rcu **stack[PEER_MAXDEPTH], ***stackptr;
> struct inet_peer_base *base = family_to_base(daddr->family);
> @@ -436,7 +436,10 @@ relookup:
> p->daddr = *daddr;
> atomic_set(&p->refcnt, 1);
> atomic_set(&p->rid, 0);
> - atomic_set(&p->ip_id_count, secure_ip_id(daddr->addr.a4));
> + atomic_set(&p->ip_id_count,
> + (daddr->family == AF_INET) ?
> + secure_ip_id(daddr->addr.a4) :
> + secure_ipv6_id(daddr->addr.a6));
> p->tcp_ts_stamp = 0;
> p->metrics[RTAX_LOCK-1] = INETPEER_METRICS_NEW;
> p->rate_tokens = 0;
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 8db0e48..32e5339 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -596,6 +596,31 @@ int ip6_find_1stfragopt(struct sk_buff *skb, u8 **nexthdr)
> return offset;
> }
>
> +void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt)
> +{
> + static atomic_t ipv6_fragmentation_id;
> + int old, new;
> +
> + if (rt) {
> + struct inet_peer *peer;
> +
> + if (!rt->rt6i_peer)
> + rt6_bind_peer(rt, 1);
> + peer = rt->rt6i_peer;
> + if (peer) {
> + fhdr->identification = htonl(inet_getid(peer, 0));
> + return;
> + }
> + }
> + do {
> + old = atomic_read(&ipv6_fragmentation_id);
> + new = old + 1;
> + if (!new)
> + new = 1;
> + } while (atomic_cmpxchg(&ipv6_fragmentation_id, old, new) != old);
> + fhdr->identification = htonl(new);
> +}
> +
> int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
> {
> struct sk_buff *frag;
> @@ -680,7 +705,7 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
> skb_reset_network_header(skb);
> memcpy(skb_network_header(skb), tmp_hdr, hlen);
>
> - ipv6_select_ident(fh);
> + ipv6_select_ident(fh, rt);
> fh->nexthdr = nexthdr;
> fh->reserved = 0;
> fh->frag_off = htons(IP6_MF);
> @@ -826,7 +851,7 @@ slow_path:
> fh->nexthdr = nexthdr;
> fh->reserved = 0;
> if (!frag_id) {
> - ipv6_select_ident(fh);
> + ipv6_select_ident(fh, rt);
> frag_id = fh->identification;
> } else
> fh->identification = frag_id;
> @@ -1076,7 +1101,8 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> int getfrag(void *from, char *to, int offset, int len,
> int odd, struct sk_buff *skb),
> void *from, int length, int hh_len, int fragheaderlen,
> - int transhdrlen, int mtu,unsigned int flags)
> + int transhdrlen, int mtu,unsigned int flags,
> + struct rt6_info *rt)
>
> {
> struct sk_buff *skb;
> @@ -1120,7 +1146,7 @@ static inline int ip6_ufo_append_data(struct sock *sk,
> skb_shinfo(skb)->gso_size = (mtu - fragheaderlen -
> sizeof(struct frag_hdr)) & ~7;
> skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> - ipv6_select_ident(&fhdr);
> + ipv6_select_ident(&fhdr, rt);
> skb_shinfo(skb)->ip6_frag_id = fhdr.identification;
> __skb_queue_tail(&sk->sk_write_queue, skb);
>
> @@ -1286,7 +1312,7 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to,
>
> err = ip6_ufo_append_data(sk, getfrag, from, length,
> hh_len, fragheaderlen,
> - transhdrlen, mtu, flags);
> + transhdrlen, mtu, flags, rt);
> if (err)
> goto error;
> return 0;
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 328985c..29213b5 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1359,7 +1359,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, u32 features)
> fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
> fptr->nexthdr = nexthdr;
> fptr->reserved = 0;
> - ipv6_select_ident(fptr);
> + ipv6_select_ident(fptr, (struct rt6_info *)skb_dst(skb));
>
> /* Fragment the skb. ipv6 header and the remaining fields of the
> * fragment header are updated in ipv6_gso_segment()
>
--
Mathematics is the supreme nostalgia of our time.
^ permalink raw reply
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods
From: Ben Greear @ 2011-07-19 21:01 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jiri Pirko, Neil Horman, netdev, Alexey Dobriyan, David S. Miller
In-Reply-To: <1311108107.3113.22.camel@edumazet-laptop>
On 07/19/2011 01:41 PM, Eric Dumazet wrote:
> Le mardi 19 juillet 2011 à 22:29 +0200, Jiri Pirko a écrit :
>
>> You are right, but it may not cause panic, right? In case this patch
>> would cause significant performance regression, how about to just forbid
>> pktgen to run on soft-netdevs ?
>>
>
> Please do
No, please do not. Pktgen *can* work on soft-devs if you do not
use multi-skb, and it can be useful for testing highs speed vlan
traffic and various other things.
If you must, forbid multi-skb on soft devices, but don't just
break pktgen for all soft-devices.
Thanks,
Ben
>
> Note : a sysadmin has other ways to make a machine panic or reboot or
> halt...
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* RE: [PATCH 00/45] Update bna driver version to 3.0.2.0
From: Rasesh Mody @ 2011-07-19 22:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org, Adapter Linux Open SRC Team
In-Reply-To: <20110718.185805.413849001475627611.davem@davemloft.net>
>From: David Miller [mailto:davem@davemloft.net]
>Sent: Monday, July 18, 2011 6:58 PM
>
>From: Rasesh Mody <rmody@brocade.com>
>Date: Mon, 18 Jul 2011 17:48:03 -0700
>
>> We wanted to avoid submitting developing code that could break
>> upstream driver due to partial changes.
>
>So what you're saying is that if only some of the patches are applied
>the driver will not work properly?
>
>That's not allowed either, your set of changes must be fully
>bisectable meaning that at any point in the series the driver
>must still compile and work properly.
>
>You guys need to sort out how you guys submit your changes.
>
>When you have 4 patches ready to go, post them immediately.
>
>Because if there are fundamental problem with the first 4,
>everything else that depends upon them will need to change.
David,
We can further split the submission into multiple smaller submissions (e.g. 4 patches a time as you suggested), but there still will be one big patch set for new hardware and feature enablement, which cannot be split it into small bisectable patches. Is this acceptable? What is the upstream guideline for submitting big changes such as driver re-architecture or re-organization?
Thanks,
--Rasesh
^ permalink raw reply
* Re: [PATCH 00/45] Update bna driver version to 3.0.2.0
From: David Miller @ 2011-07-19 23:24 UTC (permalink / raw)
To: rmody; +Cc: netdev, adapter_linux_open_src_team
In-Reply-To: <E5313AF6F2BFD14293E5FD0F94750F86A83BDF4C07@HQ1-EXCH01.corp.brocade.com>
From: Rasesh Mody <rmody@brocade.com>
Date: Tue, 19 Jul 2011 15:53:06 -0700
> We can further split the submission into multiple smaller
> submissions (e.g. 4 patches a time as you suggested), but there
> still will be one big patch set for new hardware and feature
> enablement
That's fine, just don't add the patch that causes the driver
to recognize the new device IDs until all the necessary
support code is there.
^ permalink raw reply
* Re: [PATCH] slip: remove unused 'line' field from the 'slip' structure
From: David Miller @ 2011-07-19 23:55 UTC (permalink / raw)
To: matvejchikov; +Cc: netdev
In-Reply-To: <CAKh5nabuWJtnfM6PEOkGbEog89jmiAAiPPArV==2F5mX0PUH-Q@mail.gmail.com>
From: Matvejchikov Ilya <matvejchikov@gmail.com>
Date: Tue, 19 Jul 2011 11:56:44 +0400
> Signed-off-by: Matvejchikov Ilya <matvejchikov@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] slcan: remove unused 'leased', 'line' and 'pid' fields from the 'slcan' structure
From: David Miller @ 2011-07-19 23:56 UTC (permalink / raw)
To: matvejchikov; +Cc: netdev
In-Reply-To: <CAKh5naa=x13=wUE+MLAYv0XQ4OhuY5G47ZDmFz0vbkibNtoz-A@mail.gmail.com>
From: Matvejchikov Ilya <matvejchikov@gmail.com>
Date: Tue, 19 Jul 2011 11:58:48 +0400
> Signed-off-by: Matvejchikov Ilya <matvejchikov@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods
From: Neil Horman @ 2011-07-20 0:19 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jiri Pirko, netdev, Alexey Dobriyan, David S. Miller
In-Reply-To: <1311108107.3113.22.camel@edumazet-laptop>
On Tue, Jul 19, 2011 at 10:41:47PM +0200, Eric Dumazet wrote:
> Le mardi 19 juillet 2011 à 22:29 +0200, Jiri Pirko a écrit :
>
> > You are right, but it may not cause panic, right? In case this patch
> > would cause significant performance regression, how about to just forbid
> > pktgen to run on soft-netdevs ?
> >
>
> Please do
>
You are correct Eric, this can cause a significant performance regression, but I
think that beats causing a panic or other unexpected behavior. I read your
previous threads with others regarding fixing this with vlans, but I don't think
its fair to just say 'its fast, but it might cause oopses'.
And its not sufficient to simply forbid soft drivers to make use of pktgen, its
not just a soft driver problem, its systemic. Any driver which assumes that it
has exclusive access to an skb submitted for transmit is at risk from pktgen in
its current implementation. That of course as a subset includes all the soft
drivers, but others are also suceptible. As examples (some of which I noted in
the origional post) virtio_net uses the skb->cb to hold vnet header information
which will be corrupted on sucessive sends. bnx2x linearizes skbs under certain
circumstances, which means pktgen, if it marshals a fragmented frame will not
send a fragmented frame after the first iteration. The PPP and Slip drivers
skb_push the skb to prepend a header to the frame on send, meaning sucessive
uses, up until they get an skb_under_panic will get iteratively more malformed
frames on the wire as ppp headers get stacked on top of one another. These are
ust a few of the examples I've found.
The long and the short of it in my mind, is that we have a fundamental
disconnect between driver asumptions and pktgen. If its ok to submit shared
skbs to drivers, then we need to augment drivers that modify skbs on transmit to
clone the skb (likey not an efficient solution), or if its not ok to do so, we
need to change pktgen to not behave that way.
> Note : a sysadmin has other ways to make a machine panic or reboot or
> halt...
Yes, predictable ways, that the sysadmin can see coming based on what they're
doing (i.e. no one should be shocked if they dd /dev/random to /dev/kmem and get
a hang or panic, or if they issue a sysrq-c, etc). This case is different. A
sysadmin reasonably expects pktgen to send the frames they configure on the
interface they specify. While its arguably reasonable to forsee that it may not
work with soft interfaces, pktgen just won't work with some hardware drivers (as
per the examples above). And it won't always be an oops, it may be occasional
random behvaior in the output data, and its highly dependent not just on the use
of pktgen, but rather the specific command(s) issued.
I'm sensitive to the performance impact, but I would much rather see a lower
performing pktgen that doesn't randomly crash, and bring the performance back up
in a safe, reliable way. To that end, I've been starting to think about
pre-allocating a ring buffer of skbs with a skb->users count biased up to
prevent driver freeing. That way we could detect 'unused skb's' by a user count
that was at the bias level. Thoughts?
Neil
>
>
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox