* [PATCH 1/2] af_ieee802154: fix ioctl processing
From: Dmitry Eremin-Solenikov @ 2009-08-06 12:26 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Sergey Lapin, linux-zigbee-devel
In-Reply-To: <1249561586-26927-1-git-send-email-dbaryshkov@gmail.com>
fix two errors in ioctl processing:
1) if the ioctl isn't supported one should return -ENOIOCTLCMD
2) don't call ndo_do_ioctl if the device doesn't provide it
Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
net/ieee802154/af_ieee802154.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/ieee802154/af_ieee802154.c b/net/ieee802154/af_ieee802154.c
index 3bb6bdb..af66180 100644
--- a/net/ieee802154/af_ieee802154.c
+++ b/net/ieee802154/af_ieee802154.c
@@ -136,7 +136,7 @@ static int ieee802154_dev_ioctl(struct sock *sk, struct ifreq __user *arg,
unsigned int cmd)
{
struct ifreq ifr;
- int ret = -EINVAL;
+ int ret = -ENOIOCTLCMD;
struct net_device *dev;
if (copy_from_user(&ifr, arg, sizeof(struct ifreq)))
@@ -146,8 +146,10 @@ static int ieee802154_dev_ioctl(struct sock *sk, struct ifreq __user *arg,
dev_load(sock_net(sk), ifr.ifr_name);
dev = dev_get_by_name(sock_net(sk), ifr.ifr_name);
- if (dev->type == ARPHRD_IEEE802154 ||
- dev->type == ARPHRD_IEEE802154_PHY)
+
+ if ((dev->type == ARPHRD_IEEE802154 ||
+ dev->type == ARPHRD_IEEE802154_PHY) &&
+ dev->netdev_ops->ndo_do_ioctl)
ret = dev->netdev_ops->ndo_do_ioctl(dev, &ifr, cmd);
if (!ret && copy_to_user(arg, &ifr, sizeof(struct ifreq)))
--
1.6.3.3
^ permalink raw reply related
* [PATCH 2/2] af_ieee802154: provide dummy get/setsockopt
From: Dmitry Eremin-Solenikov @ 2009-08-06 12:26 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Sergey Lapin, linux-zigbee-devel
In-Reply-To: <1249561586-26927-2-git-send-email-dbaryshkov@gmail.com>
Provide dummt get/setsockopt implementations to stop these
syscalls from oopsing on our sockets.
Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
net/ieee802154/dgram.c | 14 ++++++++++++++
net/ieee802154/raw.c | 14 ++++++++++++++
2 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/net/ieee802154/dgram.c b/net/ieee802154/dgram.c
index 14d3984..ba8b214 100644
--- a/net/ieee802154/dgram.c
+++ b/net/ieee802154/dgram.c
@@ -377,6 +377,18 @@ int ieee802154_dgram_deliver(struct net_device *dev, struct sk_buff *skb)
return ret;
}
+static int dgram_getsockopt(struct sock *sk, int level, int optname,
+ char __user *optval, int __user *optlen)
+{
+ return -EOPNOTSUPP;
+}
+
+static int dgram_setsockopt(struct sock *sk, int level, int optname,
+ char __user *optval, int __user optlen)
+{
+ return -EOPNOTSUPP;
+}
+
struct proto ieee802154_dgram_prot = {
.name = "IEEE-802.15.4-MAC",
.owner = THIS_MODULE,
@@ -391,5 +403,7 @@ struct proto ieee802154_dgram_prot = {
.connect = dgram_connect,
.disconnect = dgram_disconnect,
.ioctl = dgram_ioctl,
+ .getsockopt = dgram_getsockopt,
+ .setsockopt = dgram_setsockopt,
};
diff --git a/net/ieee802154/raw.c b/net/ieee802154/raw.c
index fca44d5..9315977 100644
--- a/net/ieee802154/raw.c
+++ b/net/ieee802154/raw.c
@@ -238,6 +238,18 @@ void ieee802154_raw_deliver(struct net_device *dev, struct sk_buff *skb)
read_unlock(&raw_lock);
}
+static int raw_getsockopt(struct sock *sk, int level, int optname,
+ char __user *optval, int __user *optlen)
+{
+ return -EOPNOTSUPP;
+}
+
+static int raw_setsockopt(struct sock *sk, int level, int optname,
+ char __user *optval, int __user optlen)
+{
+ return -EOPNOTSUPP;
+}
+
struct proto ieee802154_raw_prot = {
.name = "IEEE-802.15.4-RAW",
.owner = THIS_MODULE,
@@ -250,5 +262,7 @@ struct proto ieee802154_raw_prot = {
.unhash = raw_unhash,
.connect = raw_connect,
.disconnect = raw_disconnect,
+ .getsockopt = raw_getsockopt,
+ .setsockopt = raw_setsockopt,
};
--
1.6.3.3
^ permalink raw reply related
* [PATCH] appletalk: fix atalk_getname() leak
From: Eric Dumazet @ 2009-08-06 12:27 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
atalk_getname() can leak 8 bytes of kernel memory to user
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 590b839..9ef6ff2 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -1237,6 +1237,7 @@ static int atalk_getname(struct socket *sock, struct sockaddr *uaddr,
return -ENOBUFS;
*uaddr_len = sizeof(struct sockaddr_at);
+ memset(&sat.sat_zero, 0, sizeof(sat.sat_zero));
if (peer) {
if (sk->sk_state != TCP_ESTABLISHED)
^ permalink raw reply related
* Re: [PATCH, RFC] bonding: prevent outgoing packets on inactive slaves
From: Eric Dumazet @ 2009-08-06 12:28 UTC (permalink / raw)
To: Jiri Bohac; +Cc: fubar, davem, netdev
In-Reply-To: <20090806104356.GA8024@midget.suse.cz>
Jiri Bohac a écrit :
> On Wed, Aug 05, 2009 at 06:57:37PM +0200, Eric Dumazet wrote:
>> Jiri Bohac a écrit :
>>> This patch makes sure the TX queues on inactive slaves are
>>> deactivated.
>>>
>> Wont this break ARP link monitoring ?
>
> No, ARP monitoring sends the ARP requests from the active slave.
>
Yes, this makes sense now you say it :)
Thanks
^ permalink raw reply
* Re: [PATCH 0/7] AlacrityVM guest drivers Reply-To:
From: Avi Kivity @ 2009-08-06 12:54 UTC (permalink / raw)
To: Gregory Haskins
Cc: Michael S. Tsirkin, alacrityvm-devel, kvm, linux-kernel, netdev
In-Reply-To: <4A7A8F7B0200005A00051B84@sinclair.provo.novell.com>
On 08/06/2009 03:08 PM, Gregory Haskins wrote:
>> Merging the guest first means relying on
>> kernel interface from an out of tree driver, which well might change
>> before it goes in.
>>
>
> ABI compatibility is already addressed/handled, so even if that is true its not a problem.
>
>
Really the correct way to address the ABI is to publish a spec and write
both host and guest drivers to that. Unfortunately we didn't do this
with virtio.
It becomes more important when you have multiple implementations (e.g.
Windows drivers).
>>> This series implements the guest-side drivers for accelerated IO
>>> when running on top of the AlacrityVM hypervisor, the details of
>>> which you can find here:
>>>
>>> http://developer.novell.com/wiki/index.php/AlacrityVM
>>>
>> Since AlacrityVM is kvm based, Cc kvm@vger.kernel.org.
>>
>
> I *can* do that, but there is nothing in these drivers that is KVM specific (its all pure PCI and VBUS). I've already made the general announcement about the project/ml cross posted to KVM for anyone that might be interested, but I figure I will spare the general KVM list the details unless something specifically pertains to, or affects, KVM. For instance, when I get to pushing the hypervisor side, I still need to work on getting that 'xinterface' patch to you guys. I would certainly be CC'ing kvm@vger when that happens since it modifies the KVM code.
>
> So instead, I would just encourage anyone interested (such as yourself) to join the alacrity list so I don't bother the KVM community unless absolutely necessary.
>
It's true that vbus is a separate project (in fact even virtio is
completely separate from kvm). Still I think it would be of interest to
many kvm@ readers.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH 0/7] AlacrityVM guest drivers Reply-To:
From: Gregory Haskins @ 2009-08-06 13:00 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: alacrityvm-devel, kvm, linux-kernel, netdev
In-Reply-To: <20090806122449.GC11038@redhat.com>
>>> On 8/6/2009 at 8:24 AM, in message <20090806122449.GC11038@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Aug 06, 2009 at 06:08:27AM -0600, Gregory Haskins wrote:
>> Hi Michael,
>>
>> >>> On 8/6/2009 at 4:19 AM, in message <20090806081955.GA9752@redhat.com>,
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Mon, Aug 03, 2009 at 01:17:30PM -0400, Gregory Haskins wrote:
>> >> (Applies to v2.6.31-rc5, proposed for linux-next after review is complete)
>> >
>> > These are guest drivers, right?
>>
>> Yep.
>>
>> > Merging the guest first means relying on
>> > kernel interface from an out of tree driver, which well might change
>> > before it goes in.
>>
>> ABI compatibility is already addressed/handled, so even if that is true its
> not a problem.
>
> It is? With versioning? Presumably this:
>
> + params.devid = vdev->id;
> + params.version = version;
> +
> + ret = vbus_pci_hypercall(VBUS_PCI_HC_DEVOPEN,
> + ¶ms, sizeof(params));
> + if (ret < 0)
> + return ret;
This is part of it. There are various ABI version components (which, by the way, are only expected to only allow change while the code is experimental/alpha). The other component is capability functions (such as NEGCAP in the venet driver).
>
> Even assuming host even knows how to decode this structure (e.g. some
> other host module doesn't use VBUS_PCI_HC_DEVOPEN),
This argument demonstrates a fundamental lack of understanding on how AlacrityVM works. Please study the code more closely and you will see that your concern is illogical. If it's still not clear, let me know and I will walk it through for you.
> checks the version
> and denies older guests, this might help guest not to crash, but guest
> still won't work.
Thats ok. As I said above, the version number is just there for gross ABI protection and generally will never be changed once a driver is "official" (if at all). We use things like capability-bit negotiation to allow backwards compat.
For an example, see drivers/net/vbus-enet.c, line 703:
http://git.kernel.org/?p=linux/kernel/git/ghaskins/alacrityvm/linux-2.6.git;a=blob;f=drivers/net/vbus-enet.c;h=7220f43723adc5b0bece1bc37974fae1b034cd9e;hb=b3b2339efbd4e754b1c85f8bc8f85f21a1a1f509#l703
venet exposes a verb "NEGCAP" (negotiate capabilities), which is used to extend the ABI. The version number you quote above (on the device open) is really just a check to make sure the NEGCAP ABI is compatible. The rest of the abi is negotiated at runtime with capability feature bits.
FWIW; I decided to not built a per-device capability into the low-level vbus protocol (e.g. there is no VBUS_PCI_HC_NEGCAP) because I felt as though the individual devices could better express their own capability mechanism, rather than try to generalize it. Therefore it is up to each device to define its own mechanism, presumably using a verb from its own private call() namespace (as venet has done).
>
>> > Would it make more sense to start merging with the host side of the
> project?
>>
>> Not necessarily, no. These are drivers for a "device", so its no
>> different than merging any other driver really. This is especially
>> true since the hypervisor is also already published and freely
>> available today, so anyone can start using it.
>
> The difference is clear to me: devices do not get to set kernel/userspace
> interfaces. This "device" depends on a specific interface between
> kernel and (guest) userspace.
This doesn't really parse for me, but I think the gist of it is based on an incorrect assumption.
Can you elaborate?
Kind Regards,
-Greg
^ permalink raw reply
* Re: [PATCH 0/7] AlacrityVM guest drivers Reply-To:
From: Gregory Haskins @ 2009-08-06 13:03 UTC (permalink / raw)
To: Avi Kivity
Cc: alacrityvm-devel, Michael S. Tsirkin, kvm, linux-kernel, netdev
In-Reply-To: <4A7AD29E.50800@redhat.com>
>>> On 8/6/2009 at 8:54 AM, in message <4A7AD29E.50800@redhat.com>, Avi Kivity
<avi@redhat.com> wrote:
> On 08/06/2009 03:08 PM, Gregory Haskins wrote:
>>> Merging the guest first means relying on
>>> kernel interface from an out of tree driver, which well might change
>>> before it goes in.
>>>
>>
>> ABI compatibility is already addressed/handled, so even if that is true its
> not a problem.
>>
>>
>
> Really the correct way to address the ABI is to publish a spec and write
> both host and guest drivers to that. Unfortunately we didn't do this
> with virtio.
>
> It becomes more important when you have multiple implementations (e.g.
> Windows drivers).
>
>>>> This series implements the guest-side drivers for accelerated IO
>>>> when running on top of the AlacrityVM hypervisor, the details of
>>>> which you can find here:
>>>>
>>>> http://developer.novell.com/wiki/index.php/AlacrityVM
>>>>
>>> Since AlacrityVM is kvm based, Cc kvm@vger.kernel.org.
>>>
>>
>> I *can* do that, but there is nothing in these drivers that is KVM specific
> (its all pure PCI and VBUS). I've already made the general announcement
> about the project/ml cross posted to KVM for anyone that might be interested,
> but I figure I will spare the general KVM list the details unless something
> specifically pertains to, or affects, KVM. For instance, when I get to
> pushing the hypervisor side, I still need to work on getting that
> 'xinterface' patch to you guys. I would certainly be CC'ing kvm@vger when
> that happens since it modifies the KVM code.
>>
>> So instead, I would just encourage anyone interested (such as yourself) to
> join the alacrity list so I don't bother the KVM community unless absolutely
> necessary.
>>
>
> It's true that vbus is a separate project (in fact even virtio is
> completely separate from kvm). Still I think it would be of interest to
> many kvm@ readers.
Well, my goal was to not annoy KVM readers. ;) So if you feel as though there is benefit to having all of KVM CC'd and I won't be annoying everyone, I see no problem in cross posting.
Would you like to see all conversations, or just ones related to code (and, of course, KVM relevant items)?
Regards,
-Greg
^ permalink raw reply
* [PATCH] netrom: Fix nr_getname() leak
From: Eric Dumazet @ 2009-08-06 13:31 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
nr_getname() can leak kernel memory to user.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
index ce51ce0..ce1a34b 100644
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -847,6 +847,7 @@ static int nr_getname(struct socket *sock, struct sockaddr *uaddr,
sax->fsa_ax25.sax25_family = AF_NETROM;
sax->fsa_ax25.sax25_ndigis = 1;
sax->fsa_ax25.sax25_call = nr->user_addr;
+ memset(sax->fsa_digipeater, 0, sizeof(sax->fsa_digipeater));
sax->fsa_digipeater[0] = nr->dest_addr;
*uaddr_len = sizeof(struct full_sockaddr_ax25);
} else {
^ permalink raw reply related
* [PATCH] rose: Fix rose_getname() leak
From: Eric Dumazet @ 2009-08-06 13:34 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
rose_getname() can leak kernel memory to user.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index f0a76f6..e5f478c 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -954,6 +954,7 @@ static int rose_getname(struct socket *sock, struct sockaddr *uaddr,
struct rose_sock *rose = rose_sk(sk);
int n;
+ memset(srose, 0, sizeof(*srose));
if (peer != 0) {
if (sk->sk_state != TCP_ESTABLISHED)
return -ENOTCONN;
^ permalink raw reply related
* Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()
From: Herbert Xu @ 2009-08-06 13:37 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Paul Moore, netdev, David Miller
In-Reply-To: <m1hbwlkzt6.fsf@fess.ebiederm.org>
On Thu, Aug 06, 2009 at 03:21:41AM -0700, Eric W. Biederman wrote:
>
> Two threads one file descriptor. Both simultaneously attempt to
> attach to a tun device. One will fail, the other succeed.
>
> At least that is how I read the locking.
Yes but the "race" fixed by this patch is centred on the tun_attach
call for a newly created network device. As tun_set_iff occurs
under RTNL, the second thread cannot start attaching until the
creation thread has completed. IOW the thread that creates the
net device should always succeed in attaching.
If two threads try to attach to the same device that was already
created then yes one will fail and the other succeed. However,
AFAICS that case has nothing to do with this patch.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 0/7] AlacrityVM guest drivers Reply-To:
From: Avi Kivity @ 2009-08-06 13:44 UTC (permalink / raw)
To: Gregory Haskins
Cc: alacrityvm-devel, Michael S. Tsirkin, kvm, linux-kernel, netdev
In-Reply-To: <4A7A9C6B0200005A00051BB9@sinclair.provo.novell.com>
On 08/06/2009 04:03 PM, Gregory Haskins wrote:
>> It's true that vbus is a separate project (in fact even virtio is
>> completely separate from kvm). Still I think it would be of interest to
>> many kvm@ readers.
>>
>
> Well, my goal was to not annoy KVM readers. ;) So if you feel as though there is benefit to having all of KVM CC'd and I won't be annoying everyone, I see no problem in cross posting.
>
I can only speak for myself, I'm interested in this project (though
still rooting for virtio).
> Would you like to see all conversations, or just ones related to code (and, of course, KVM relevant items)
I guess internal vbus changes won't be too interesting for most readers,
but new releases, benchmarks, and kvm-related stuff will be welcome on
the kvm list.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH 0/7] AlacrityVM guest drivers Reply-To:
From: Gregory Haskins @ 2009-08-06 13:45 UTC (permalink / raw)
To: Avi Kivity
Cc: alacrityvm-devel, Michael S. Tsirkin, kvm, linux-kernel, netdev
In-Reply-To: <4A7ADE23.5010208@redhat.com>
>>> On 8/6/2009 at 9:44 AM, in message <4A7ADE23.5010208@redhat.com>, Avi Kivity
<avi@redhat.com> wrote:
> On 08/06/2009 04:03 PM, Gregory Haskins wrote:
>>> It's true that vbus is a separate project (in fact even virtio is
>>> completely separate from kvm). Still I think it would be of interest to
>>> many kvm@ readers.
>>>
>>
>> Well, my goal was to not annoy KVM readers. ;) So if you feel as though
> there is benefit to having all of KVM CC'd and I won't be annoying everyone,
> I see no problem in cross posting.
>>
>
> I can only speak for myself, I'm interested in this project
In that case, the best solution is probably to have you (and anyone else interested) to sign up, then:
https://lists.sourceforge.net/lists/listinfo/alacrityvm-devel
https://lists.sourceforge.net/lists/listinfo/alacrityvm-users
> (though still rooting for virtio).
Heh...not to belabor the point to death, but virtio is orthogonal (you keep forgetting that ;).
Its really the vbus device-model vs the qemu device-model (and possibly vs the "in-kernel pci emulation" model that I believe Michael is working on).
You can run virtio on any of those three.
>
>> Would you like to see all conversations, or just ones related to code (and,
> of course, KVM relevant items)
>
> I guess internal vbus changes won't be too interesting for most readers,
> but new releases, benchmarks, and kvm-related stuff will be welcome on
> the kvm list.
Ok, I was planning on that anyway.
Regards,
-Greg
^ permalink raw reply
* [PATCH] econet: Fix econet_getname() leak
From: Eric Dumazet @ 2009-08-06 13:48 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
econet_getname() can leak kernel memory to user.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/econet/af_econet.c b/net/econet/af_econet.c
index e114da7..0e0254f 100644
--- a/net/econet/af_econet.c
+++ b/net/econet/af_econet.c
@@ -520,6 +520,7 @@ static int econet_getname(struct socket *sock, struct sockaddr *uaddr,
if (peer)
return -EOPNOTSUPP;
+ memset(sec, 0, sizeof(*sec));
mutex_lock(&econet_mutex);
sk = sock->sk;
^ permalink raw reply related
* [PATCH] irda: Fix irda_getname() leak
From: Eric Dumazet @ 2009-08-06 13:55 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
irda_getname() can leak kernel memory to user.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c
index cb762c8..3ec2b43 100644
--- a/net/irda/af_irda.c
+++ b/net/irda/af_irda.c
@@ -714,6 +714,7 @@ static int irda_getname(struct socket *sock, struct sockaddr *uaddr,
struct sock *sk = sock->sk;
struct irda_sock *self = irda_sk(sk);
+ memset(&saddr, 0, sizeof(saddr));
if (peer) {
if (sk->sk_state != TCP_ESTABLISHED)
return -ENOTCONN;
^ permalink raw reply related
* Re: [PATCH 1/7] shm-signal: shared-memory signals
From: Arnd Bergmann @ 2009-08-06 13:56 UTC (permalink / raw)
To: Gregory Haskins; +Cc: linux-kernel, alacrityvm-devel, netdev
In-Reply-To: <20090803171735.17268.37490.stgit@dev.haskins.net>
On Monday 03 August 2009, Gregory Haskins wrote:
> shm-signal provides a generic shared-memory based bidirectional
> signaling mechanism. It is used in conjunction with an existing
> signal transport (such as posix-signals, interrupts, pipes, etc) to
> increase the efficiency of the transport since the state information
> is directly accessible to both sides of the link. The shared-memory
> design provides very cheap access to features such as event-masking
> and spurious delivery mititgation, and is useful implementing higher
> level shared-memory constructs such as rings.
Looks like a very useful feature in general.
> +struct shm_signal_irq {
> + __u8 enabled;
> + __u8 pending;
> + __u8 dirty;
> +};
Won't this layout cause cache line ping pong? Other schemes I have
seen try to separate the bits so that each cache line is written to
by only one side. This gets much more interesting if the two sides
are on remote ends of an I/O link, e.g. using a nontransparent
PCI bridge, where you only want to send stores over the wire, but
never fetches or even read-modify-write cycles.
Your code is probably optimal if you only communicate between host
and guest code on the same CPU, but not so good if it crosses NUMA
nodes or worse.
> +struct shm_signal_desc {
> + __u32 magic;
> + __u32 ver;
> + struct shm_signal_irq irq[2];
> +};
This data structure has implicit padding of two bytes at the end.
How about adding another '__u16 reserved' to make it explicit?
> + /*
> + * We always mark the remote side as dirty regardless of whether
> + * they need to be notified.
> + */
> + irq->dirty = 1;
> + wmb(); /* dirty must be visible before we test the pending state */
> +
> + if (irq->enabled && !irq->pending) {
> + rmb();
> +
> + /*
> + * If the remote side has enabled notifications, and we do
> + * not see a notification pending, we must inject a new one.
> + */
> + irq->pending = 1;
> + wmb(); /* make it visible before we do the injection */
> +
> + s->ops->inject(s);
> + }
Barriers always confuse me, but the rmb() looks slightly wrong. AFAIU
it only prevents reads after the barrier from being done before the
barrier, but you don't do any reads after it.
The (irq->enabled && !irq->pending) check could be done before the
irq->dirty = 1 arrives at the bus, but that does not seem to hurt, it
would at most cause a duplicate ->inject().
Regarding the scope of the barrier, did you intentionally use the
global versions (rmb()/wmb()) and not the lighter single-system
(smp_rmb()/smp_wmb()) versions? Your version should cope with remote
links over PCI but looks otherwise optimized for local use, as I
wrote above.
Arnd <><
^ permalink raw reply
* Re: [PATCH 0/7] AlacrityVM guest drivers Reply-To:
From: Avi Kivity @ 2009-08-06 13:57 UTC (permalink / raw)
To: Gregory Haskins
Cc: alacrityvm-devel, Michael S. Tsirkin, kvm, linux-kernel, netdev
In-Reply-To: <4A7AA63A0200005A00051BE2@sinclair.provo.novell.com>
On 08/06/2009 04:45 PM, Gregory Haskins wrote:
>
>> (though still rooting for virtio).
>>
>
> Heh...not to belabor the point to death, but virtio is orthogonal (you keep forgetting that ;).
>
> Its really the vbus device-model vs the qemu device-model (and possibly vs the "in-kernel pci emulation" model that I believe Michael is working on).
>
> You can run virtio on any of those three.
>
It's not orthogonal. virtio is one set of ABI+guest drivers+host
support to get networking on kvm guests. AlacrityVM's vbus-based
drivers are another set of ABI+guest drivers+host support to get
networking on kvm guests. That makes them competitors (two different
ways to do one thing), not orthogonal.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH 0/7] AlacrityVM guest drivers Reply-To:
From: Michael S. Tsirkin @ 2009-08-06 13:59 UTC (permalink / raw)
To: Gregory Haskins; +Cc: Avi Kivity, alacrityvm-devel, kvm, linux-kernel, netdev
In-Reply-To: <4A7AA63A0200005A00051BE2@sinclair.provo.novell.com>
On Thu, Aug 06, 2009 at 07:45:30AM -0600, Gregory Haskins wrote:
> > (though still rooting for virtio).
>
> Heh...not to belabor the point to death, but virtio is orthogonal (you keep forgetting that ;).
venet and virtio aren't orthogonal, are they?
--
MST
^ permalink raw reply
* Re: [PATCH 0/7] AlacrityVM guest drivers Reply-To:
From: Gregory Haskins @ 2009-08-06 14:06 UTC (permalink / raw)
To: Avi Kivity
Cc: alacrityvm-devel, Michael S. Tsirkin, kvm, linux-kernel, netdev
In-Reply-To: <4A7AE150.7040009@redhat.com>
>>> On 8/6/2009 at 9:57 AM, in message <4A7AE150.7040009@redhat.com>, Avi Kivity
<avi@redhat.com> wrote:
> On 08/06/2009 04:45 PM, Gregory Haskins wrote:
>>
>>> (though still rooting for virtio).
>>>
>>
>> Heh...not to belabor the point to death, but virtio is orthogonal (you keep
> forgetting that ;).
>>
>> Its really the vbus device-model vs the qemu device-model (and possibly vs the
> "in-kernel pci emulation" model that I believe Michael is working on).
>>
>> You can run virtio on any of those three.
>>
>
> It's not orthogonal. virtio is one set of ABI+guest drivers+host
> support to get networking on kvm guests. AlacrityVM's vbus-based
> drivers are another set of ABI+guest drivers+host support to get
> networking on kvm guests. That makes them competitors (two different
> ways to do one thing), not orthogonal.
Thats not accurate, though.
The virtio stack is modular. For instance, with virtio-net, you have
(guest-side)
|--------------------------
| virtio-net
|--------------------------
| virtio-ring
|--------------------------
| virtio-bus
|--------------------------
| virtio-pci
|--------------------------
|
(pci)
|
|--------------------------
| kvm.ko
|--------------------------
| qemu
|--------------------------
| tun-tap
|--------------------------
| netif
|--------------------------
(host-side)
We can exchange out the "virtio-pci" module like this:
(guest-side)
|--------------------------
| virtio-net
|--------------------------
| virtio-ring
|--------------------------
| virtio-bus
|--------------------------
| virtio-vbus
|--------------------------
| vbus-proxy
|--------------------------
| vbus-connector
|--------------------------
|
(vbus)
|
|--------------------------
| kvm.ko
|--------------------------
| vbus-connector
|--------------------------
| vbus
|--------------------------
| virtio-net-tap (vbus model)
|--------------------------
| netif
|--------------------------
(host-side)
So virtio-net runs unmodified. What is "competing" here is "virtio-pci" vs "virtio-vbus". Also, venet vs virtio-net are technically competing. But to say "virtio vs vbus" is inaccurate, IMO.
HTH
-Greg
^ permalink raw reply
* Re: [PATCH 0/7] AlacrityVM guest drivers Reply-To:
From: Gregory Haskins @ 2009-08-06 14:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: alacrityvm-devel, Avi Kivity, kvm, linux-kernel, netdev
In-Reply-To: <20090806135903.GA11530@redhat.com>
>>> On 8/6/2009 at 9:59 AM, in message <20090806135903.GA11530@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Aug 06, 2009 at 07:45:30AM -0600, Gregory Haskins wrote:
>> > (though still rooting for virtio).
>>
>> Heh...not to belabor the point to death, but virtio is orthogonal (you keep
> forgetting that ;).
>
> venet and virtio aren't orthogonal, are they?
See my last reply to Avi.
Regards,
-Greg
^ permalink raw reply
* Re: Low latency diagnostic tools
From: Christoph Lameter @ 2009-08-06 14:19 UTC (permalink / raw)
To: Mark Smith; +Cc: netdev
In-Reply-To: <20090806094736.591b7086.lk-netdev@lk-netdev.nosense.org>
On Thu, 6 Aug 2009, Mark Smith wrote:
> This implementation of One Way Active Measurement Protocol might be of
> interest:
>
> http://www.internet2.edu/performance/owamp/
It needs an accurate timesource. So it wont be of general use unless
everyone adopts ptp or so. Latencies today are way below 100 microseconds
which is not doable with the accuracy provided by ntp.
> Some of the performance tuning parts of the README below would also be
> useful in the Net area of the Linux Foundation wiki. Possibly the
> "Testing" section could be changed to "Testing and Measurement"
>
> http://www.linuxfoundation.org/en/Net:Main_Page
Ok. I will look at that.
^ permalink raw reply
* Re: [RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver
From: Paul Moore @ 2009-08-06 14:24 UTC (permalink / raw)
To: Serge E. Hallyn; +Cc: eparis, netdev, linux-security-module, selinux
In-Reply-To: <20090806021558.GA17998@us.ibm.com>
On Wednesday 05 August 2009 10:15:58 pm Serge E. Hallyn wrote:
> Quoting Paul Moore (paul.moore@hp.com):
> > On Wednesday 05 August 2009 10:13:50 am Serge E. Hallyn wrote:
> > > Quoting Paul Moore (paul.moore@hp.com):
> >
> > [NOTE: my email has been out all day due to some mysterious FS issue so
> > my apologies for not replying sooner]
> >
> > ...
> >
> > > The checks before and after this patch are not equivalent. Post-patch,
> > > one must always have CAP_NET_ADMIN to do the attach, whereas pre-patch
> > > you only needed those if current_cred() did not own the tun device. Is
> > > that intentional?
> >
> > Nope, just a goof on my part; I misread the booleans and haven't fully
> > tested the patch yet so it slipped out, thanks for catching it. This
> > brings up a good point, would we rather move the TUN owner/group checks
> > into the cap_tun_* functions or move the capable() call back into the TUN
> > driver? The answer wasn't clear to me when I was looking at the code
> > before and the uniqueness of the TUN driver doesn't help much in this
> > regard.
>
> I see the question being asked as: Does this device belong to
> the caller and, if not, is the caller privileged to act
> anyway?' So I think the capable call should be moved back
> into the tun driver, followed by a separate security_tun_dev_attach()
> check, since that is a separate, restrictive question.
Works for me, I'll make the change.
BTW, the main reason for posting the patches in such an early state was to
solicit feedback on the location and types of hooks added; I've read lots of
good feedback but nothing regarding the fundamental aspects of the hooks ...
any comments before I push out v2?
--
paul moore
linux @ hp
^ permalink raw reply
* Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()
From: Eric W. Biederman @ 2009-08-06 14:27 UTC (permalink / raw)
To: Herbert Xu; +Cc: Paul Moore, netdev, David Miller
In-Reply-To: <20090806133746.GA28557@gondor.apana.org.au>
Herbert Xu <herbert@gondor.apana.org.au> writes:
> On Thu, Aug 06, 2009 at 03:21:41AM -0700, Eric W. Biederman wrote:
>>
>> Two threads one file descriptor. Both simultaneously attempt to
>> attach to a tun device. One will fail, the other succeed.
>>
>> At least that is how I read the locking.
>
> Yes but the "race" fixed by this patch is centred on the tun_attach
> call for a newly created network device. As tun_set_iff occurs
> under RTNL, the second thread cannot start attaching until the
> creation thread has completed. IOW the thread that creates the
> net device should always succeed in attaching.
>
> If two threads try to attach to the same device that was already
> created then yes one will fail and the other succeed. However,
> AFAICS that case has nothing to do with this patch.
Summarizing:
tun = __tun_get(tfile);
if (!tun) { // No tun we are not attached.
< -------------------- race opportunity
rtnl_lock();
tun_set_iff();
rtnl_unlock();
}
...
We don't test if we are attached under the rtnl
until we get to tun_attach();
So two threads can both do:
tun = __tun_get(tfile);
if (!tun) {
rtnl_lock();
tun_set_iff();
dev = __dev_get_by_name(net, "not_an_interface_name");
if (!dev) {
dev = alloc_netdev(....);
...;
register_netdev(dev);
...;
err = tun_attach(..);
}
Only one thread is in tun_set_iff() at a time, but the other thread
could have attached the file to a device before the one in tun_attach().
Eric
^ permalink raw reply
* Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()
From: Herbert Xu @ 2009-08-06 14:39 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Paul Moore, netdev, David Miller
In-Reply-To: <m1ljlxj9vi.fsf@fess.ebiederm.org>
On Thu, Aug 06, 2009 at 07:27:13AM -0700, Eric W. Biederman wrote:
>
> Summarizing:
>
> tun = __tun_get(tfile);
> if (!tun) { // No tun we are not attached.
> < -------------------- race opportunity
> rtnl_lock();
> tun_set_iff();
> rtnl_unlock();
> }
> ...
>
> We don't test if we are attached under the rtnl
> until we get to tun_attach();
>
> So two threads can both do:
>
> tun = __tun_get(tfile);
> if (!tun) {
> rtnl_lock();
> tun_set_iff();
> dev = __dev_get_by_name(net, "not_an_interface_name");
> if (!dev) {
> dev = alloc_netdev(....);
> ...;
> register_netdev(dev);
> ...;
> err = tun_attach(..);
> }
>
>
> Only one thread is in tun_set_iff() at a time, but the other thread
> could have attached the file to a device before the one in tun_attach().
Right, I see what you mean. However I don't think this is possible
because the ioctl runs under the big kernel lock.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 4/7] vbus-proxy: add a pci-to-vbus bridge
From: Arnd Bergmann @ 2009-08-06 14:42 UTC (permalink / raw)
To: Gregory Haskins; +Cc: linux-kernel, alacrityvm-devel, netdev
In-Reply-To: <20090803171751.17268.48169.stgit@dev.haskins.net>
On Monday 03 August 2009, Gregory Haskins wrote:
> This patch adds a pci-based driver to interface between the a host VBUS
> and the guest's vbus-proxy bus model.
>
> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
This seems to be duplicating parts of virtio-pci that could be kept
common by extending the virtio code. Layering on top of virtio
would also make it possible to use the same features you add
on top of other transports (e.g. the s390 virtio code) without
adding yet another backend for each of them.
> +static int
> +vbus_pci_hypercall(unsigned long nr, void *data, unsigned long len)
> +{
> + struct vbus_pci_hypercall params = {
> + .vector = nr,
> + .len = len,
> + .datap = __pa(data),
> + };
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&vbus_pci.lock, flags);
> +
> + memcpy_toio(&vbus_pci.regs->hypercall.data, ¶ms, sizeof(params));
> + ret = ioread32(&vbus_pci.regs->hypercall.result);
> +
> + spin_unlock_irqrestore(&vbus_pci.lock, flags);
> +
> + return ret;
> +}
The functionality looks reasonable but please don't call this a hypercall.
A hypercall would be hypervisor specific by definition while this one
is device specific if I understand it correctly. How about "command queue",
"mailbox", "message queue", "devcall" or something else that we have in
existing PCI devices?
> +
> +static int
> +vbus_pci_device_open(struct vbus_device_proxy *vdev, int version, int flags)
> +{
> + struct vbus_pci_device *dev = to_dev(vdev);
> + struct vbus_pci_deviceopen params;
> + int ret;
> +
> + if (dev->handle)
> + return -EINVAL;
> +
> + params.devid = vdev->id;
> + params.version = version;
> +
> + ret = vbus_pci_hypercall(VBUS_PCI_HC_DEVOPEN,
> + ¶ms, sizeof(params));
> + if (ret < 0)
> + return ret;
> +
> + dev->handle = params.handle;
> +
> + return 0;
> +}
This seems to add an artificial abstraction that does not make sense
if you stick to the PCI abstraction. The two sensible and common models
for virtual devices that I've seen are:
* The hypervisor knows what virtual resources exist and provides them
to the guest. The guest owns them as soon as they show up in the
bus (e.g. PCI) probe. The 'handle' is preexisting.
* The guest starts without any devices and asks for resources it wants
to access. There is no probing of resources but the guest issues
a hypercall to get a handle to a newly created virtual device
(or -ENODEV).
What is your reasoning for requiring both a probe and an allocation?
> +static int
> +vbus_pci_device_shm(struct vbus_device_proxy *vdev, int id, int prio,
> + void *ptr, size_t len,
> + struct shm_signal_desc *sdesc, struct shm_signal **signal,
> + int flags)
> +{
> + struct vbus_pci_device *dev = to_dev(vdev);
> + struct _signal *_signal = NULL;
> + struct vbus_pci_deviceshm params;
> + unsigned long iflags;
> + int ret;
> +
> + if (!dev->handle)
> + return -EINVAL;
> +
> + params.devh = dev->handle;
> + params.id = id;
> + params.flags = flags;
> + params.datap = (u64)__pa(ptr);
> + params.len = len;
> +
> + if (signal) {
> + /*
> + * The signal descriptor must be embedded within the
> + * provided ptr
> + */
> + if (!sdesc
> + || (len < sizeof(*sdesc))
> + || ((void *)sdesc < ptr)
> + || ((void *)sdesc > (ptr + len - sizeof(*sdesc))))
> + return -EINVAL;
> +
> + _signal = kzalloc(sizeof(*_signal), GFP_KERNEL);
> + if (!_signal)
> + return -ENOMEM;
> +
> + _signal_init(&_signal->signal, sdesc, &_signal_ops);
> +
> + /*
> + * take another reference for the host. This is dropped
> + * by a SHMCLOSE event
> + */
> + shm_signal_get(&_signal->signal);
> +
> + params.signal.offset = (u64)sdesc - (u64)ptr;
> + params.signal.prio = prio;
> + params.signal.cookie = (u64)_signal;
> +
> + } else
> + params.signal.offset = -1; /* yes, this is a u32, but its ok */
> +
> + ret = vbus_pci_hypercall(VBUS_PCI_HC_DEVSHM,
> + ¶ms, sizeof(params));
> + if (ret < 0) {
> + if (_signal) {
> + /*
> + * We held two references above, so we need to drop
> + * both of them
> + */
> + shm_signal_put(&_signal->signal);
> + shm_signal_put(&_signal->signal);
> + }
> +
> + return ret;
> + }
> +
> + if (signal) {
> + _signal->handle = ret;
> +
> + spin_lock_irqsave(&vbus_pci.lock, iflags);
> +
> + list_add_tail(&_signal->list, &dev->shms);
> +
> + spin_unlock_irqrestore(&vbus_pci.lock, iflags);
> +
> + shm_signal_get(&_signal->signal);
> + *signal = &_signal->signal;
> + }
> +
> + return 0;
> +}
This could be implemented by virtio devices as well, right?
> +static int
> +vbus_pci_device_call(struct vbus_device_proxy *vdev, u32 func, void *data,
> + size_t len, int flags)
> +{
> + struct vbus_pci_device *dev = to_dev(vdev);
> + struct vbus_pci_devicecall params = {
> + .devh = dev->handle,
> + .func = func,
> + .datap = (u64)__pa(data),
> + .len = len,
> + .flags = flags,
> + };
> +
> + if (!dev->handle)
> + return -EINVAL;
> +
> + return vbus_pci_hypercall(VBUS_PCI_HC_DEVCALL, ¶ms, sizeof(params));
> +}
Why the indirection? It seems to me that you could do the simpler
static int
vbus_pci_device_call(struct vbus_device_proxy *vdev, u32 func, void *data,
size_t len, int flags)
{
struct vbus_pci_device *dev = to_dev(vdev);
struct vbus_pci_hypercall params = {
.vector = func,
.len = len,
.datap = __pa(data),
};
spin_lock_irqsave(&dev.lock, flags);
memcpy_toio(&dev.regs->hypercall.data, ¶ms, sizeof(params));
ret = ioread32(&dev.regs->hypercall.result);
spin_unlock_irqrestore(&dev.lock, flags);
return ret;
}
This gets rid of your 'handle' and the unwinding through an extra pointer
indirection. You just need to make sure that the device specific call numbers
don't conflict with any global ones.
> +
> +static struct ioq_notifier eventq_notifier;
> ...
> +/* Invoked whenever the hypervisor ioq_signal()s our eventq */
> +static void
> +eventq_wakeup(struct ioq_notifier *notifier)
> +{
> + struct ioq_iterator iter;
> + int ret;
> +
> + /* We want to iterate on the head of the in-use index */
> + ret = ioq_iter_init(&vbus_pci.eventq, &iter, ioq_idxtype_inuse, 0);
> + BUG_ON(ret < 0);
> +
> + ret = ioq_iter_seek(&iter, ioq_seek_head, 0, 0);
> + BUG_ON(ret < 0);
> +
> + /*
> + * The EOM is indicated by finding a packet that is still owned by
> + * the south side.
> + *
> + * FIXME: This in theory could run indefinitely if the host keeps
> + * feeding us events since there is nothing like a NAPI budget. We
> + * might need to address that
> + */
> + while (!iter.desc->sown) {
> + struct ioq_ring_desc *desc = iter.desc;
> + struct vbus_pci_event *event;
> +
> + event = (struct vbus_pci_event *)desc->cookie;
> +
> + switch (event->eventid) {
> + case VBUS_PCI_EVENT_DEVADD:
> + event_devadd(&event->data.add);
> + break;
> + case VBUS_PCI_EVENT_DEVDROP:
> + event_devdrop(&event->data.handle);
> + break;
> + case VBUS_PCI_EVENT_SHMSIGNAL:
> + event_shmsignal(&event->data.handle);
> + break;
> + case VBUS_PCI_EVENT_SHMCLOSE:
> + event_shmclose(&event->data.handle);
> + break;
> + default:
> + printk(KERN_WARNING "VBUS_PCI: Unexpected event %d\n",
> + event->eventid);
> + break;
> + };
> +
> + memset(event, 0, sizeof(*event));
> +
> + /* Advance the in-use head */
> + ret = ioq_iter_pop(&iter, 0);
> + BUG_ON(ret < 0);
> + }
> +
> + /* And let the south side know that we changed the queue */
> + ioq_signal(&vbus_pci.eventq, 0);
> +}
Ah, so you have a global event queue and your own device hotplug mechanism.
But why would you then still use PCI to back it? We already have PCI hotplug
to add and remove devices and you have defined per device notifier queues
that you can use for waking up the device, right?
Arnd <><
^ permalink raw reply
* Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()
From: Eric W. Biederman @ 2009-08-06 15:02 UTC (permalink / raw)
To: Herbert Xu; +Cc: Paul Moore, netdev, David Miller
In-Reply-To: <20090806143959.GA29323@gondor.apana.org.au>
Herbert Xu <herbert@gondor.apana.org.au> writes:
> On Thu, Aug 06, 2009 at 07:27:13AM -0700, Eric W. Biederman wrote:
>>
>> Summarizing:
>>
>> tun = __tun_get(tfile);
>> if (!tun) { // No tun we are not attached.
>> < -------------------- race opportunity
>> rtnl_lock();
>> tun_set_iff();
>> rtnl_unlock();
>> }
>> ...
>>
>> We don't test if we are attached under the rtnl
>> until we get to tun_attach();
>>
>> So two threads can both do:
>>
>> tun = __tun_get(tfile);
>> if (!tun) {
>> rtnl_lock();
>> tun_set_iff();
>> dev = __dev_get_by_name(net, "not_an_interface_name");
>> if (!dev) {
>> dev = alloc_netdev(....);
>> ...;
>> register_netdev(dev);
>> ...;
>> err = tun_attach(..);
>> }
>>
>>
>> Only one thread is in tun_set_iff() at a time, but the other thread
>> could have attached the file to a device before the one in tun_attach().
>
> Right, I see what you mean. However I don't think this is possible
> because the ioctl runs under the big kernel lock.
Why not? We can sleep on that code path.
Although now that you mention it we should use unlocked_ioctl unless
we actually need the BKL.
Eric
^ 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