* Re: [PATCH][RFC] net/bridge: add basic VEPA support
From: Stephen Hemminger @ 2009-08-07 4:00 UTC (permalink / raw)
To: Fischer, Anna
Cc: bridge@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, virtualization@lists.linux-foundation.org,
evb@yahoogroups.com, davem@davemloft.net, kaber@trash.net,
adobriyan@gmail.com, Arnd Bergmann, Paul Congdon (UC Davis)
In-Reply-To: <0199E0D51A61344794750DC57738F58E67D2DCECBB@GVW1118EXC.americas.hpqcorp.net>
On Mon, 15 Jun 2009 17:33:10 +0000
"Fischer, Anna" <anna.fischer@hp.com> wrote:
> This patch adds basic Virtual Ethernet Port Aggregator (VEPA)
> capabilities to the Linux kernel Ethernet bridging code.
>
> A Virtual Ethernet Port Aggregator (VEPA) is a capability within
> a physical end station that collaborates with an adjacent, external
> bridge to provide distributed bridging support between multiple
> virtual end stations and external networks. The VEPA collaborates
> by forwarding all station-originated frames to the adjacent bridge
> for frame processing and frame relay (including so-called 'hairpin'
> forwarding) and by steering and replicating frames received from
> the VEPA uplink to the appropriate destinations. A VEPA may be
> implemented in software or in conjunction with embedded hardware.
>
> In particular, the patch extends the Linux Ethernet bridge to act as
> (1) a VEPA - for this we have added VEPA forwarding functionality and
> added a configuration option for a VEPA uplink port, or as
> (2) a bridge supporting 'hairpin' forwarding - for this we have added a
> bridge port 'hairpin' mode which allows sending frames back out
> through the port the frame was received on.
>
> Configuration of VEPA capabilities through Linux userspace bridge
> utilities is provided by an additional patch 'bridge-utils: add
> basic VEPA support'.
After reading more about this, I am not convinced this should be part
of the bridge code. The bridge code really consists of two parts:
forwarding table and optional spanning tree. Well the VEPA code
short circuits both of these; it can't imagine it working
with STP turned on. The only part of bridge code that really gets
used by this are the receive packet hooks and the crufty old
API.
So instead of adding more stuff to existing bridge code, why not
have a new driver for just VEPA. You could
do it with a simple version of macvlan type driver.
^ permalink raw reply
* Re: [patch for 2.6.31? 2/2] gigaset: really fix chars_in_buffer
From: Greg KH @ 2009-08-07 3:50 UTC (permalink / raw)
To: David Miller; +Cc: akpm, netdev, tilman
In-Reply-To: <20090806.202227.224933454.davem@davemloft.net>
On Thu, Aug 06, 2009 at 08:22:27PM -0700, David Miller wrote:
> From: akpm@linux-foundation.org
> Date: Thu, 06 Aug 2009 15:19:32 -0700
>
> > From: Tilman Schmidt <tilman@imap.cc>
> >
> > The tty_operation chars_in_buffer() is not allowed to return a negative
> > value to signal an error. Corrects the problem flagged by commit
> > 23198fda7182969b619613a555f8645fdc3dc334, "tty: fix chars_in_buffers".
> >
> > Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>
> I was letting Alan Cox handle changes in this area since it's
> TTY stuff, and thus it probably should go to the new TTY
> mad man, CC:'d :-)
Thanks, I'll queue it up.
greg k-h
^ permalink raw reply
* Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()
From: Eric W. Biederman @ 2009-08-07 4:22 UTC (permalink / raw)
To: David Miller; +Cc: herbert, paul.moore, netdev
In-Reply-To: <20090806.204032.39020854.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Fri, 7 Aug 2009 10:22:44 +1000
>
>> tun: Extend RTNL lock coverage over whole ioctl
>>
>> As it is, parts of the ioctl runs under the RTNL and parts of
>> it do not. The unlocked section is still protected by the BKL,
>> but there can be subtle races. For example, Eric Biederman and
>> Paul Moore observed that if two threads tried to create two tun
>> devices on the same file descriptor, then unexpected results
>> may occur.
>>
>> As there isn't anything in the ioctl that is expected to sleep
>> indefinitely, we can prevent this from occurring by extending
>> the RTNL lock coverage.
>>
>> This also allows to get rid of the BKL.
>>
>> Finally, I changed tun_get_iff to take a tun device in order to
>> avoid calling tun_put which would dead-lockt as it also tries to
>> take the RTNL lock.
>>
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> This looks good after a quick audit, Eric what say you?
Looks good to me.
Eric
^ permalink raw reply
* Re: [PATCH 4/7] vbus-proxy: add a pci-to-vbus bridge
From: Gregory Haskins @ 2009-08-07 4:42 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: alacrityvm-devel, Ira W. Snyder, linux-kernel, netdev
In-Reply-To: <200908070057.54795.arnd@arndb.de>
>>> On 8/6/2009 at 6:57 PM, in message <200908070057.54795.arnd@arndb.de>, Arnd
Bergmann <arnd@arndb.de> wrote:
> On Thursday 06 August 2009, Gregory Haskins wrote:
>> >>> On 8/6/2009 at 1:03 PM, in message <200908061903.05083.arnd@arndb.de>, Arnd
> Bergmann <arnd@arndb.de> wrote:
>> Here are some of my arguments against it:
>>
>> 1) there is an ample PCI model that is easy to work with when you are in
> QEMU and using its device model (and you get it for free). Its the path of
> least resistance. For something in kernel, it is more awkward to try to
> coordinate the in-kernel state with the PCI state. Afaict, you either need to
> have it live partially in both places, or you need some PCI emulation in the
> kernel.
>
> True, if the whole hypervisor is in the host kernel, then doing full PCI
> emulation would be
> insane.
In this case, the entire bus is more or less self contained and in-kernel. We technically *do* still have qemu present, however, so you are right there.
> I was assuming that all of the setup code still lived in host user
> space.
Very little. Just enough to register the PCI device, handle the MMIO/PIO/MSI configuration, etc. All the bus management uses the standard vbus management interface (configfs/sysfs)
> What is the reason why it cannot?
Its not that it "can't" per se.. Its just awkward to have it live in two places, and I would need to coordinate in-kernel changes to userspace, etc. Today I do not need to do this: i.e. the model in userspace is very simple.
> Do you want to use something other than qemu,
Well, only in the sense that vbus has its own management interface and bus model, and I want them to be used.
> do you think this will impact performance, or something else?
performance is not a concern for this aspect of operation.
>
>> 2) The signal model for the 1:1 design is not very flexible IMO.
>> 2a) I want to be able to allocate dynamic signal paths, not pre-allocate
> msi-x vectors at dev-add.
>
> I believe msi-x implies that the interrupt vectors get added by the device
> driver
> at run time, unlike legacy interrupts or msi. It's been a while since I
> dealt with
> that though.
Yeah, its been a while for me too. I would have to look at the spec again.
My understanding was that its just a slight variation of msi, with some of the constraints revised (no <= 32 vector limit, etc). Perhaps it is fancier than that and 2a is unfounded. TBD.
>
>> 2b) I also want to collapse multiple interrupts together so as to
> minimize the context switch rate (inject + EIO overhead). My design
> effectively has "NAPI" for interrupt handling. This helps when the system
> needs it the most: heavy IO.
>
> That sounds like a very useful concept in general, but this seems to be a
> detail of the interrupt controller implementation. If the IO-APIC cannot
> do what you want here, maybe we just need a paravirtual IRQ controller
> driver, like e.g. the PS3 has.
Yeah, I agree this could be a function of the APIC code. Do note that I mentioned this in passing to Avi a few months ago but FWIW he indicated at that time that he is not interested in making the APIC PV.
Also, I almost forgot an important one. Add:
2c) Interrupt prioritization. I want to be able to assign priority to interrupts and handle them in priority order.
>
>> 3) The 1:1 model is not buying us much in terms of hotplug. We don't really
> "use" PCI very much even in virtio. Its a thin-shim of uniform dev-ids to
> resurface to the virtio-bus as something else. With LDM, hotplug is
> ridiculously easy anyway, so who cares. I already need an event channel
> anyway for (2b) anyway, so the devadd/devdrop events are trivial to handle.
>
> I agree for Linux guests, but when you want to run other guest operating
> systems,
> PCI hotplug is probably the most common interface for this. AFAIK, the
> windows
> virtio-net driver does not at all have a concept of a virtio layer but is
> simply
> a network driver for a PCI card. The same could be applied any other device,
> possibly with some library code doing all the queue handling in a common
> way.l
I was told it also has a layering like Linux, but I haven't actually seen the code myself, so I do not know if this is true.
>
>> 4) communicating with something efficiently in-kernel requires more finesse
> than basic PIO/MMIO. There are tricks you can do to get around this, but
> with 1:1 you would have to do this trick repeatedly for each device. Even
> with a library solution to help, you still have per-cpu .data overhead and cpu
> hotplug overhead to get maximum performance. With my "bridge" model, I do it
> once, which I believe is ideal.
>>
>> 5) 1:1 is going to quickly populate the available MMIO/PIO and IDT slots for
> any kind of medium to large configuration. The bridge model scales better in
> this regard.
>
> We don't need to rely on PIO, it's just the common interface that all
> hypervisors
> can easily support. We could have different underlying methods for the
> communication
> if space or performance becomes a bottleneck because of this.
Heh...I already proposed an alternative, which incidentally was shot down:
http://lkml.org/lkml/2009/5/5/132
(in the end, I think we agreed that a technique of tunneling PIO/MMIO over hypercaills would be better than introducing a new namespace. But we also decided that the difference between PIO and PIOoHC was too small to care, and we don't care about non-x86)
But in any case, my comment still stands: 1:1 puts load on the PIO emulation (even if you use PIOoHC). I am not sure this can be easily worked around.
>
>> So based on that, I think the bridge model works better for vbus. Perhaps
> you can convince me otherwise ;)
>
> Being able to define all of it in the host kernel seems to be the major
> advantage of your approach, the other points you mentioned are less
> important IMHO. The question is whether that is indeed a worthy goal,
> or if it should just live in user space as with the qemu PCI code.
I don't think we can gloss over these so easily. They are all important to me, particularly 2b and 2c.
>
>> >> In essence, this driver's job is to populate the "vbus-proxy" LDM bus with
>> >> objects that it finds across the PCI-OTHER bridge. This would actually sit
>> >> below the virtio components in the stack, so it doesnt make sense (to me)
> to
>> >> turn around and build this on top of virtio. But perhaps I am missing
>> >> something you are seeing.
>> >>
>> >> Can you elaborate?
>> >
>> > Your PCI device does not serve any real purpose as far as I can tell
>>
>> That is certainly debatable. Its purpose is as follows:
>>
>> 1) Allows a guest to discover the vbus feature (fwiw: I used to do this with
> cpuid)
>
> true, I missed that.
>
>> 2) Allows the guest to establish proper context to communicate with the
> feature (mmio, pio, and msi) (fwiw: i used to use hypercalls)
>> 3) Access the virtual-devices that have been configured for the feature
>>
>> Correct me if I am wrong: Isn't this more of less the exact intent of
> something like an LDM bus (vbus-proxy) and a PCI-BRIDGE? Other than the
> possibility that there might be some mergable overlap (still debatable), I
> don't think its fair to say that this does not serve a purpose.
>
> I guess you are right on that. An interesting variation of that would be
> make the
> child devices of it virtio devices again though: Instead of the PCI
> emulation code
> in the host kernel, you could define a simpler interface to the same effect.
> So the
> root device would be a virtio-pci device, below which you can have
> virtio-virtio
> devices.
Interesting....but note I think that is effectively what I do today (with virtio-vbus) except you wouldn't have the explicit vbus-proxy model underneath. Also, if 1:1 via PCI is important for windows, that solution would have the same problem that the virtio-vbus model does.
>
>> >, you could just as well have a root device as a parent for all the vbus
> devices
>> > if you do your device probing like this.
>>
>> Yes, I suppose the "bridge" could have been advertised as a virtio-based root
> device. In this way, the virtio probe() would replace my pci probe() for
> feature discovery, and a virtqueue could replace my msi+ioq for the eventq
> channel.
>>
>> I see a few issues with that, however:
>>
>> 1) The virtqueue library, while a perfectly nice ring design at the metadata
> level, does not have an API that is friendly to kernel-to-kernel communication.
> It was designed more for frontend use to some remote backend. The IOQ
> library on the other hand, was specifically designed to support use as
> kernel-to-kernel (see north/south designations). So this made life easier
> for me. To do what you propose, the eventq channel would need to terminate
> in kernel, and I would thus be forced to deal that the potential API
> problems.
>
> Well, virtqueues are not that bad for kernel-to-kernel communication, as Ira
> mentioned
> referring to his virtio-over-PCI driver. You can have virtqueues on both
> sides, having
> the host kernel create a pair of virtqueues (one in user aka guest space,
> one in the host
> kernel), with the host virtqueue_ops doing copy_{to,from}_user to move data
> between them.
Its been a while since I looked, so perhaps I am wrong here. I will look again.
>
> If you have that, you can actually use the same virtio_net driver in both
> guest and
> host kernel, just communicating over different virtio implementations.
> Interestingly,
> that would mean that you no longer need a separation between guest and host
> device
> drivers (vbus and vbus-proxy in your case) but could use the same device
> abstraction
> with just different transports to back the shm-signal or virtqueue.
Actually, I think there are some problems with that model (such as management of the interface). virtio-net really wants to connect to a virtio-net-backend (such as the one in qemu or vbus). It wasn't designed to connect back to back like that. I think you will quickly run into problems similar to what Ira faced with virtio-over-PCI with that model.
>
>> 2) I would need to have Avi et. al. allocate a virtio vector to use from
> their namespace, which I am sure they wont be willing to do until they accept
> my design. Today, I have a nice conflict free PCI ID to use as I see fit.
>
> My impression is the opposite: as long as you try to reinvent everything at
> once,
> you face opposition, but if you just improve parts of the existing design
> one
> by one (like eventfd), I think you will find lots of support.
>
>> Im sure both of these hurdles are not insurmountable, but I am left
> scratching my head as to why its worth the effort. It seems to me its a "six
> of one, half-dozen of the other" kind of scenario. Either I write a qemu PCI
> device and pci-bridge driver, or I write a qemu virtio-devicve and virtio root
> driver.
>>
>> In short: What does this buy us, or did you mean something else?
>
> In my last reply, I was thinking of a root device that can not be probed
> like a PCI device.
IIUC, because you missed the "feature discovery" function of the bridge, you thought this was possible but now see it is problematic? Or are you saying that this concept is still valid and should be considered? I think its the former, but wanted to be sure we were on the same page.
>
>> > However, assuming that you do the IMHO right thing to do probing like
>> > virtio with a PCI device for each slave, the code will be almost the same
>> > as virtio-pci and the two can be the same.
>>
>> Can you elaborate?
>
> Well, let me revise based on the discussion:
>
> The main point that remains is that I think a vbus-proxy should be the same
> as a
> virtio device. This could be done by having (as in my earlier mails) a PCI
> device
> per vbus-proxy, with devcall implemented in PIO or config-space and additional
> shm/shm-signal,
So the problem with this model is the points I made earlier (such as 2b, 2c).
I do agree with you that the *lack* of this model may be problematic for Windows, depending on the answer w.r.t. what the windows drivers look like.
> or it could be a single virtio device from virtio-pci or one
> of the other existing provides that connects you with a new virtio provider
> sitting in the host kernel. This provider has child devices for any endpoint
> (virtio-net, venet, ...) that is implemented in the host kernel.
This is an interesting idea, but I think it also has problems.
What we do get with having the explicit vbus-proxy exposed in the stack (aside from being able to support "vbus native" drivers, like venet) is a neat way to map vbus-isms into virtio-isms. For instance, vbus uses a string-based device id, and virtio uses a PCI-ID. Using this as an intermediate layer allows the "virtio" vbus-id to know that we issue dev->call(GETID) to obtain the PCI-ID value of this virtio-device, and we should publish this result to virtio-bus.
Without this intermediate layer, the vbus identity scheme would have to be compatible with virtio PCI-ID based scheme, and I think this is suboptimal for the overall design of vbus.
[snip]
>
>> Regarding the id->handle indirection:
>>
>> Internally, the DEVOPEN call translates an "id" to a "handle". The handle
> is just a token to help ensure that the caller actually opened the device
> successfully. Note that the "id" namespace is 0 based. Therefore, something
> like an errant DEVCALL(0) would be indistinguishable from a legit request.
> Using the handle abstraction gives me a slightly more robust mechanism to
> ensure the caller actually meant to call the host, and was in the proper
> context to do so. For one thing, if the device had never been opened, this
> would have failed before it ever reached the model. Its one more check I can
> do at the infrastructure level, and one less thing each model has to look out
> for.
>>
>> Is the id->handle translation critical? No, i'm sure we could live without
> it, but I also don't think it hurts anything. It allows the overall code to
> be slightly more robust, and the individual model code to be slightly less
> complicated. Therefore, I don't see a problem.
>
> Right, assuming your model with all vbus devices behind a single PCI device,
> your
> handle does not hurt, it's the equivalent of a bus/dev/fn number or an MMIO
> address.
Agreed
Thanks Arnd,
-Greg
^ permalink raw reply
* Re: [PATCH V2 net-next-2.6] net: relax dst refcnt in input path
From: Eric Dumazet @ 2009-08-07 5:05 UTC (permalink / raw)
To: David Miller; +Cc: kaber, netdev, markmc
In-Reply-To: <20090806.133518.263376332.davem@davemloft.net>
David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 22 Jul 2009 17:40:06 +0200
>
>> [PATCH net-next-2.6] net: relax dst refcnt in input path
>
>
> Two things:
>
> 1) Don't use a boolean name like "noref" it results in using
> double-negatives in one's mind while trying to read and understand
> the code.
>
> Call these arguments "need_ref" or something like that.
>
> Also, use "bool" type.
Sure
>
> 2) I wonder about this:
>
>> @@ -1700,9 +1700,15 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>> * If device doesnt need skb->dst, release it right now while
>> * its hot in this cpu cache
>> */
>> - if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
>> - skb_dst_drop(skb);
>> -
>> + if (skb_dst(skb)) {
>> + if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
>> + skb_dst_drop(skb);
>> + else
>> + /*
>> + * make sure dst was refcounted by caller
>> + */
>> + WARN_ON(!(skb->_skb_dst & SKB_DST_REFTAKEN));
>> + }
>> rc = ops->ndo_start_xmit(skb, dev);
>> if (rc == NETDEV_TX_OK)
>
> So, won't this warning trigger if we are forwarding to a device that
> does not set IFF_XMIT_DST_RELEASE?
If this device has a queue, we do a skb_dst_force() in dev_queue_xmit()
>
> If I understand things correctly, in IPv4 when we're not delivering to
> a socket, we don't take a reference.
yes, that would be the trick
>
> We'll get here from the forwarding path with a DST, and the target TX
> device has IFF_XMIT_DST_RELEASE clear, the WARN_ON above will trigger.
I added this warning to make sure I called skb_dst_force(skb)
from dev_queue_xmit() if device has a queue (packet might be queued
and thus escape from rcu lock section)
I suppose I should also skb_dst_force() if device has no queue and
has IFF_XMIT_DST_RELEASE cleared. Or should audit all these devices
and insert the skb_dst_force() if the packet must be queued in a driver queue,
and its dst needed later.
I'll respin patch anyway since commit bbd8a0d3a3b65d341437f8b99c828fa5cc29c739
(net: Avoid enqueuing skb for default qdiscs)
changed too many things, when I'll come back from vacations in two weeks.
Or if you prefer, I might submit it today (my vacations start tomorrow), but
please dont apply it while I cannot react to any bug report :)
Thanks a lot
^ permalink raw reply
* Re: [PATCH V2 net-next-2.6] net: relax dst refcnt in input path
From: David Miller @ 2009-08-07 5:09 UTC (permalink / raw)
To: eric.dumazet; +Cc: kaber, netdev, markmc
In-Reply-To: <4A7BB622.2070007@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 07 Aug 2009 07:05:38 +0200
> I'll respin patch anyway since commit bbd8a0d3a3b65d341437f8b99c828fa5cc29c739
> (net: Avoid enqueuing skb for default qdiscs)
> changed too many things, when I'll come back from vacations in two weeks.
That's fine.
> Or if you prefer, I might submit it today (my vacations start tomorrow), but
> please dont apply it while I cannot react to any bug report :)
Take your time, there is no rush.
^ permalink raw reply
* [PATCH net-2.6] can: Fix raw_getname() leak
From: Eric Dumazet @ 2009-08-07 6:27 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List, Oliver Hartkopp
raw_getname() can leak 10 bytes of kernel memory to user
(two bytes hole between can_family and can_ifindex,
8 bytes at the end of sockaddr_can structure)
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/can/raw.c b/net/can/raw.c
index f4cc445..db3152d 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -401,6 +401,7 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
if (peer)
return -EOPNOTSUPP;
+ memset(addr, 0, sizeof(*addr));
addr->can_family = AF_CAN;
addr->can_ifindex = ro->ifindex;
^ permalink raw reply related
* Re: [PATCH net-2.6] can: Fix raw_getname() leak
From: Oliver Hartkopp @ 2009-08-07 6:31 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Linux Netdev List
In-Reply-To: <4A7BC938.8010504@gmail.com>
Eric Dumazet wrote:
> raw_getname() can leak 10 bytes of kernel memory to user
>
> (two bytes hole between can_family and can_ifindex,
> 8 bytes at the end of sockaddr_can structure)
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Oliver Hartkopp <oliver@hartkopp.net>
Thanks Eric!
> ---
> diff --git a/net/can/raw.c b/net/can/raw.c
> index f4cc445..db3152d 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -401,6 +401,7 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
> if (peer)
> return -EOPNOTSUPP;
>
> + memset(addr, 0, sizeof(*addr));
> addr->can_family = AF_CAN;
> addr->can_ifindex = ro->ifindex;
>
^ permalink raw reply
* Re: [Socketcan-users] [PATCH] CAN: make checking in can_rcv less restrictive
From: Rémi Denis-Courmont @ 2009-08-07 6:52 UTC (permalink / raw)
To: ext Oliver Hartkopp, netdev@vger.kernel.org
Cc: Luotao Fu, socketcan-users@lists.berlios.de, Michael Olbrich
In-Reply-To: <4A7B0957.5020808@hartkopp.net>
Moving to netdev....
On Thursday 06 August 2009 19:48:23 ext Oliver Hartkopp wrote:
> The CAN applications can rely on getting proper CAN frames with this check.
> It was introduced some time ago together with several other sanity checks -
> even on the TX path.
>
> The CAN core *only* consumes skbuffs originated from a CAN netdevice
> (ARPHRD_CAN).
>
> When this BUG() triggers, someone provided a definitely broken *CAN*
> network driver, and this needs to be fixed on that level. It is really not
> that problematic to ensure proper CAN frames on driver level ... this
> sanity check should not be needed to be performed by every single
> application.
AFAIK, the TUN driver can inject layer-2 frames of any type, any size and any
content from userspace into the packet type handler. Sure enough, you need
CAP_NET_ADMIN and r/w access to /dev/net/tun but is it sufficient to bring the
system down?
--
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki
^ permalink raw reply
* [PATCH] Fix xfrm hash collisions by changing __xfrm4_daddr_saddr_hash to hash addresses with addition
From: Jussi Maki @ 2009-08-07 7:38 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20090806.104034.186857842.davem@davemloft.net>
This patch fixes hash collisions in cases where number
of entries have incrementing IP source and destination addresses
from single respective subnets (i.e. 192.168.0.1-172.16.0.1,
192.168.0.2-172.16.0.2, and so on.).
Signed-off-by: Jussi Maki <joamaki@gmail.com>
---
net/xfrm/xfrm_hash.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/xfrm/xfrm_hash.h b/net/xfrm/xfrm_hash.h
index d401dc8..e5195c9 100644
--- a/net/xfrm/xfrm_hash.h
+++ b/net/xfrm/xfrm_hash.h
@@ -16,7 +16,7 @@ static inline unsigned int __xfrm6_addr_hash(xfrm_address_t *addr)
static inline unsigned int __xfrm4_daddr_saddr_hash(xfrm_address_t *daddr, xfrm_address_t *saddr)
{
- return ntohl(daddr->a4 ^ saddr->a4);
+ return ntohl(daddr->a4 + saddr->a4);
}
static inline unsigned int __xfrm6_daddr_saddr_hash(xfrm_address_t *daddr, xfrm_address_t *saddr)
--
1.5.6.5
^ permalink raw reply related
* [PATCH] f_phonet: lock-less MTU change
From: Rémi Denis-Courmont @ 2009-08-07 7:56 UTC (permalink / raw)
To: netdev
From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
With the current driver, the MTU is purely indicative, so there is no
need to synchronize with the receive path.
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
drivers/usb/gadget/f_phonet.c | 14 ++------------
1 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
index d17f108..f4eff7c 100644
--- a/drivers/usb/gadget/f_phonet.c
+++ b/drivers/usb/gadget/f_phonet.c
@@ -261,20 +261,10 @@ out:
static int pn_net_mtu(struct net_device *dev, int new_mtu)
{
- struct phonet_port *port = netdev_priv(dev);
- unsigned long flags;
- int err = -EBUSY;
-
if ((new_mtu < PHONET_MIN_MTU) || (new_mtu > PHONET_MAX_MTU))
return -EINVAL;
-
- spin_lock_irqsave(&port->lock, flags);
- if (!netif_carrier_ok(dev)) {
- dev->mtu = new_mtu;
- err = 0;
- }
- spin_unlock_irqrestore(&port->lock, flags);
- return err;
+ dev->mtu = new_mtu;
+ return 0;
}
static const struct net_device_ops pn_netdev_ops = {
--
1.6.0.4
^ permalink raw reply related
* [PATCH] f_phonet: use page-sized rather than MTU-sized RX buffers
From: Rémi Denis-Courmont @ 2009-08-07 7:56 UTC (permalink / raw)
To: netdev
In-Reply-To: <1249631804-7438-1-git-send-email-remi.denis-courmont@nokia.com>
From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Instead of a large (physically) linear buffer, we generate a set of
paged sk_buff, so no extra memory copy is involved. This removes
high-order allocations and saves quite a bit of memory. Phonet MTU is
65541 bytes, so the two buffers were padded to 128 kilo-bytes each.
Now, we create 17 page buffers, almost a 75% memory use reduction.
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
drivers/usb/gadget/f_phonet.c | 77 +++++++++++++++++++++++++++--------------
1 files changed, 51 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
index f4eff7c..d2de10b 100644
--- a/drivers/usb/gadget/f_phonet.c
+++ b/drivers/usb/gadget/f_phonet.c
@@ -35,6 +35,10 @@
#include "u_phonet.h"
#define PN_MEDIA_USB 0x1B
+#define MAXPACKET 512
+#if (PAGE_SIZE % MAXPACKET)
+#error MAXPACKET must divide PAGE_SIZE!
+#endif
/*-------------------------------------------------------------------------*/
@@ -45,6 +49,10 @@ struct phonet_port {
struct f_phonet {
struct usb_function function;
+ struct {
+ struct sk_buff *skb;
+ spinlock_t lock;
+ } rx;
struct net_device *dev;
struct usb_ep *in_ep, *out_ep;
@@ -52,7 +60,7 @@ struct f_phonet {
struct usb_request *out_reqv[0];
};
-static int phonet_rxq_size = 2;
+static int phonet_rxq_size = 17;
static inline struct f_phonet *func_to_pn(struct usb_function *f)
{
@@ -138,7 +146,7 @@ pn_hs_sink_desc = {
.bEndpointAddress = USB_DIR_OUT,
.bmAttributes = USB_ENDPOINT_XFER_BULK,
- .wMaxPacketSize = cpu_to_le16(512),
+ .wMaxPacketSize = cpu_to_le16(MAXPACKET),
};
static struct usb_endpoint_descriptor
@@ -298,21 +306,21 @@ static void pn_net_setup(struct net_device *dev)
static int
pn_rx_submit(struct f_phonet *fp, struct usb_request *req, gfp_t gfp_flags)
{
- struct sk_buff *skb;
- const size_t size = fp->dev->mtu;
+ struct net_device *dev = fp->dev;
+ struct page *page;
int err;
- skb = alloc_skb(size, gfp_flags);
- if (!skb)
+ page = __netdev_alloc_page(dev, gfp_flags);
+ if (!page)
return -ENOMEM;
- req->buf = skb->data;
- req->length = size;
- req->context = skb;
+ req->buf = page_address(page);
+ req->length = PAGE_SIZE;
+ req->context = page;
err = usb_ep_queue(fp->out_ep, req, gfp_flags);
if (unlikely(err))
- dev_kfree_skb_any(skb);
+ netdev_free_page(dev, page);
return err;
}
@@ -320,25 +328,37 @@ static void pn_rx_complete(struct usb_ep *ep, struct usb_request *req)
{
struct f_phonet *fp = ep->driver_data;
struct net_device *dev = fp->dev;
- struct sk_buff *skb = req->context;
+ struct page *page = req->context;
+ struct sk_buff *skb;
+ unsigned long flags;
int status = req->status;
switch (status) {
case 0:
- if (unlikely(!netif_running(dev)))
- break;
- if (unlikely(req->actual < 1))
+ spin_lock_irqsave(&fp->rx.lock, flags);
+ skb = fp->rx.skb;
+ if (!skb)
+ skb = fp->rx.skb = netdev_alloc_skb(dev, 12);
+ if (req->actual < req->length) /* Last fragment */
+ fp->rx.skb = NULL;
+ spin_unlock_irqrestore(&fp->rx.lock, flags);
+
+ if (unlikely(!skb))
break;
- skb_put(skb, req->actual);
- skb->protocol = htons(ETH_P_PHONET);
- skb_reset_mac_header(skb);
- __skb_pull(skb, 1);
- skb->dev = dev;
- dev->stats.rx_packets++;
- dev->stats.rx_bytes += skb->len;
-
- netif_rx(skb);
- skb = NULL;
+ skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, 0,
+ req->actual);
+ page = NULL;
+
+ if (req->actual < req->length) { /* Last fragment */
+ skb->protocol = htons(ETH_P_PHONET);
+ skb_reset_mac_header(skb);
+ pskb_pull(skb, 1);
+ skb->dev = dev;
+ dev->stats.rx_packets++;
+ dev->stats.rx_bytes += skb->len;
+
+ netif_rx(skb);
+ }
break;
/* Do not resubmit in these cases: */
@@ -356,8 +376,8 @@ static void pn_rx_complete(struct usb_ep *ep, struct usb_request *req)
break;
}
- if (skb)
- dev_kfree_skb_any(skb);
+ if (page)
+ netdev_free_page(dev, page);
if (req)
pn_rx_submit(fp, req, GFP_ATOMIC);
}
@@ -375,6 +395,10 @@ static void __pn_reset(struct usb_function *f)
usb_ep_disable(fp->out_ep);
usb_ep_disable(fp->in_ep);
+ if (fp->rx.skb) {
+ dev_kfree_skb_irq(fp->rx.skb);
+ fp->rx.skb = NULL;
+ }
}
static int pn_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
@@ -573,6 +597,7 @@ int __init phonet_bind_config(struct usb_configuration *c)
fp->function.set_alt = pn_set_alt;
fp->function.get_alt = pn_get_alt;
fp->function.disable = pn_disconnect;
+ spin_lock_init(&fp->rx.lock);
err = usb_add_function(c, &fp->function);
if (err)
--
1.6.0.4
^ permalink raw reply related
* Re: 3x59x-fix-pci-resource-management.patch
From: Steffen Klassert @ 2009-08-07 8:19 UTC (permalink / raw)
To: Andrew Morton; +Cc: Sergei Shtylyov, netdev
In-Reply-To: <20090806154223.b9b9759e.akpm@linux-foundation.org>
On Thu, Aug 06, 2009 at 03:42:23PM -0700, Andrew Morton wrote:
>
> Guys, I seem to have been sitting on the below patch for, err, three years.
>
> I generally do this because there's some issue which needs to be
> addressed, but the patch didn't address it right.
>
> Can we please have a revisit and work out what we should do with this?
>
I don't know the story from the beginning. Are there any issues with
this patch aside the compile error that was caused if CONFIG_PCI is not
enabled? The compile error should be fixed and the rest looks ok on the
first view.
^ permalink raw reply
* Re: 3x59x-fix-pci-resource-management.patch
From: Andrew Morton @ 2009-08-07 8:33 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Sergei Shtylyov, netdev
In-Reply-To: <20090807081928.GA12932@skl-net.de>
On Fri, 7 Aug 2009 10:19:28 +0200 Steffen Klassert <klassert@mathematik.tu-chemnitz.de> wrote:
> On Thu, Aug 06, 2009 at 03:42:23PM -0700, Andrew Morton wrote:
> >
> > Guys, I seem to have been sitting on the below patch for, err, three years.
> >
> > I generally do this because there's some issue which needs to be
> > addressed, but the patch didn't address it right.
> >
> > Can we please have a revisit and work out what we should do with this?
> >
>
> I don't know the story from the beginning.
I've forgotten. I don't recall seeing any bug reports which this patch
might fix.
> Are there any issues with
> this patch aside the compile error that was caused if CONFIG_PCI is not
> enabled? The compile error should be fixed and the rest looks ok on the
> first view.
I just did a PCI=n, EISA=y build and it compiled OK.
^ permalink raw reply
* Re: [PATCH] be2net: Implementation of request_firmware interface.
From: Ram Pai @ 2009-08-07 9:20 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Sarveshwar Bandi, netdev, davem
In-Reply-To: <1246803604.3898.6.camel@deadeye>
On Sun, 2009-07-05 at 15:20 +0100, Ben Hutchings wrote:
> On Sun, 2009-07-05 at 17:46 +0530, Sarveshwar Bandi wrote:
> > I understand that most drivers use request_firmware() to load volatile
> > firmware. I do see that there are other nic drivers that use this inferface to
> > flash persistent firmware.
> >
> > We have other tools for offline flashing; but there is requirement
> > to flash f/w through driver without having to use other proprietary tools.
>
> The firmware blob is proprietary and has to be distributed separately
> from the kernel. So does it really matter that you have to distribute a
> special tool as well?
>
> (Based on requirements specified by major OEMs, I have implemented
> firmware update through the sfc driver (MDIO and MTD interfaces) but
> under the control of a separate tool.)
>
> > Since the firmware load happens only when there is a version mismatch with
> > f/w in /lib/firmware, Users who want to avoid automatic flashing at boot time
> > can choose not to copy the f/w file under /lib/firmware.
> [...]
>
> Is there a way of loading the firmware into the controller's RAM but not
> writing it to flash? That ought to be the default behaviour.
>
Given that the volatile and non-volatile firmware reside in the same
file, it is not possible for the driver to selectively load the intended
firmware.
However, is this behavior a gating factor for this patch from being
accepted?
RP
> Ben.
>
--
Ram Pai
System X Device-Driver Enablement Lead
Linux Technology Center
Beaverton OR-97006
503-5783752 t/l 7753752
linuxram@us.ibm.com
^ permalink raw reply
* Use correct NET_RX_* returns for atalk_rcv()
From: Mark Smith @ 2009-08-07 9:21 UTC (permalink / raw)
To: acme; +Cc: netdev
In all rx'd SKB cases, atalk_rcv() either eventually jumps to or falls through
to the label out:, which returns numeric 0. Numeric 0 corresponds to
NET_RX_SUCCESS, which is incorrect in failed SKB cases.
This patch makes atalk_rcv() provide the correct returns by:
o explicitly returning NET_RX_SUCCESS in the two success cases
o having the out: label return NET_RX_DROP, instead of numeric 0
o making the failed SKB labels and processing more consistent with other
_rcv() routines in the kernel, simplifying validation and removing a
backwards goto
Signed-off-by: Mark Smith <markzzzsmith@yahoo.com.au>
diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 590b839..2377ebe 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -1398,7 +1398,7 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
__u16 len_hops;
if (!net_eq(dev_net(dev), &init_net))
- goto freeit;
+ goto drop;
/* Don't mangle buffer if shared */
if (!(skb = skb_share_check(skb, GFP_ATOMIC)))
@@ -1406,7 +1406,7 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
/* Size check and make sure header is contiguous */
if (!pskb_may_pull(skb, sizeof(*ddp)))
- goto freeit;
+ goto drop;
ddp = ddp_hdr(skb);
@@ -1424,7 +1424,7 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
if (skb->len < sizeof(*ddp) || skb->len < (len_hops & 1023)) {
pr_debug("AppleTalk: dropping corrupted frame (deh_len=%u, "
"skb->len=%u)\n", len_hops & 1023, skb->len);
- goto freeit;
+ goto drop;
}
/*
@@ -1434,7 +1434,7 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
if (ddp->deh_sum &&
atalk_checksum(skb, len_hops & 1023) != ddp->deh_sum)
/* Not a valid AppleTalk frame - dustbin time */
- goto freeit;
+ goto drop;
/* Check the packet is aimed at us */
if (!ddp->deh_dnet) /* Net 0 is 'this network' */
@@ -1447,7 +1447,7 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
* AppleTalk iface
*/
atalk_route_packet(skb, dev, ddp, len_hops, origlen);
- goto out;
+ return NET_RX_SUCCESS;
}
/* if IP over DDP is not selected this code will be optimized out */
@@ -1463,18 +1463,21 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
sock = atalk_search_socket(&tosat, atif);
if (!sock) /* But not one of our sockets */
- goto freeit;
+ goto drop;
/* Queue packet (standard) */
skb->sk = sock;
if (sock_queue_rcv_skb(sock, skb) < 0)
- goto freeit;
-out:
- return 0;
-freeit:
+ goto drop;
+
+ return NET_RX_SUCCESS;
+
+drop:
kfree_skb(skb);
- goto out;
+out:
+ return NET_RX_DROP;
+
}
/*
^ permalink raw reply related
* [PATCH] e1000e: fix use of pci_enable_pcie_error_reporting
From: Xiaotian Feng @ 2009-08-07 9:36 UTC (permalink / raw)
To: john.ronciak, peter.p.waskiewicz.jr, bruce.w.allan,
jesse.brandeburg, jeffrey.t.kirsher, davem
Cc: e1000-devel, netdev, linux-kernel, Xiaotian Feng
commit 111b9dc5 introduces pcie aer support for e1000e, but it is not
reasonable to disable it in e1000_remove but enable it in e1000_resume.
This patch enables aer support in e1000_probe.
Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
---
drivers/net/e1000e/netdev.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 63415bb..e2f0304 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4670,14 +4670,6 @@ static int e1000_resume(struct pci_dev *pdev)
return err;
}
- /* AER (Advanced Error Reporting) hooks */
- err = pci_enable_pcie_error_reporting(pdev);
- if (err) {
- dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
- "0x%x\n", err);
- /* non-fatal, continue */
- }
-
pci_set_master(pdev);
pci_enable_wake(pdev, PCI_D3hot, 0);
@@ -4990,6 +4982,14 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
if (err)
goto err_pci_reg;
+ /* AER (Advanced Error Reporting) hooks */
+ err = pci_enable_pcie_error_reporting(pdev);
+ if (err) {
+ dev_err(&pdev->dev, "pci_enable_pcie_error_reporting failed "
+ "0x%x\n", err);
+ /* non-fatal, continue */
+ }
+
pci_set_master(pdev);
/* PCI config space info */
err = pci_save_state(pdev);
--
1.6.2.5
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
^ permalink raw reply related
* Re: 3x59x-fix-pci-resource-management.patch
From: Steffen Klassert @ 2009-08-07 10:01 UTC (permalink / raw)
To: Andrew Morton; +Cc: Sergei Shtylyov, netdev
In-Reply-To: <20090806154223.b9b9759e.akpm@linux-foundation.org>
On Thu, Aug 06, 2009 at 03:42:23PM -0700, Andrew Morton wrote:
> /* PCI-only startup logic */
> if (pdev) {
> - /* EISA resources already marked, so only PCI needs to do this here */
> - /* Ignore return value, because Cardbus drivers already allocate for us */
> - if (request_region(dev->base_addr, vci->io_size, print_name) != NULL)
> - vp->must_free_region = 1;
> -
I don't know what cardbus does, but __request_region() returns NULL on
memory allocation failure for struct resource too. So this did not work
in all cases anyway. If cardbus allocates for us, we should handle it in
some way, if not I'm fine with the patch as it is.
^ permalink raw reply
* Re: some bug in iproute2
From: Jarek Poplawski @ 2009-08-07 10:12 UTC (permalink / raw)
To: Sergey Popov; +Cc: netdev, jamal
In-Reply-To: <20090806115035.42a22dfc@azure>
On 06-08-2009 10:50, Sergey Popov wrote:
> # tc f add dev eth0 parent 1: proto ip prio 2 u32 match u32 0 0 action ipt -j MARK --set-mark 1
> /usr/lib64/iptables/libipt_mark.so: cannot open shared object file: No such file or directory
> failed to find target MARK
>
> bad action parsing
> parse_action: bad value (5:ipt)!
> Illegal "action"
>
>
> But mark target is compiled in kernel (not a module)
>
> # iptables -t mangle -A PREROUTING -i eth1 -j MARK --set-mark 1
> # iptables -t mangle -L PREROUTING
> Chain PREROUTING (policy ACCEPT)
> target prot opt source destination
> MARK all -- anywhere anywhere MARK xset
> 0x1/0xffffffff
>
> This shouldn't be.
If you're using iptables > 1.4.2 then it's a known problem.
You can read more in a netdev thread:
Subject: iproute2 action/policer question
starting date: Tue, 09 Jun 2009 22:10:46 +0200
Jarek P.
^ permalink raw reply
* Forgot "[PATCH]" - Re: Use correct NET_RX_* returns for atalk_rcv()
From: Mark Smith @ 2009-08-07 10:34 UTC (permalink / raw)
To: Mark Smith; +Cc: acme, netdev
In-Reply-To: <20090807185122.30f022de.lk-netdev@lk-netdev.nosense.org>
On Fri, 7 Aug 2009 18:51:22 +0930
Mark Smith <lk-netdev@lk-netdev.nosense.org> wrote:
>
> In all rx'd SKB cases, atalk_rcv() either eventually jumps to or falls through
> to the label out:, which returns numeric 0. Numeric 0 corresponds to
> NET_RX_SUCCESS, which is incorrect in failed SKB cases.
<snip>
^ permalink raw reply
* Re: 3x59x-fix-pci-resource-management.patch
From: Sergei Shtylyov @ 2009-08-07 10:53 UTC (permalink / raw)
To: Andrew Morton; +Cc: Steffen Klassert, netdev
In-Reply-To: <20090806154223.b9b9759e.akpm@linux-foundation.org>
Hello.
Andrew Morton wrote:
> Guys, I seem to have been sitting on the below patch for, err, three years.
> I generally do this because there's some issue which needs to be
> addressed, but the patch didn't address it right.
In this case it was different: you just wanted the patch to be verifed
on an EISA card...
On Mon, 14 Aug 2006 14:31:46 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:
>> ACK, shall I apply this to netdev#upstream ?
Spose so, but let's not go mainline until I've had a chance to test an
EISA card and a cardbus card.
There was several more comments from you but they all got resolved
between you and Jeff at the time...
> Can we please have a revisit and work out what we should do with this?
> Thanks.
WBR, Sergei
^ permalink raw reply
* Re: [PATCH][RFC] net/bridge: add basic VEPA support
From: Arnd Bergmann @ 2009-08-07 11:29 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Fischer, Anna, bridge@lists.linux-foundation.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
virtualization@lists.linux-foundation.org, evb@yahoogroups.com,
davem@davemloft.net, kaber@trash.net, adobriyan@gmail.com,
Paul Congdon (UC Davis), Or Gerlitz
In-Reply-To: <20090806210002.75beb7c6@nehalam>
On Friday 07 August 2009, Stephen Hemminger wrote:
> So instead of adding more stuff to existing bridge code, why not
> have a new driver for just VEPA. You could
> do it with a simple version of macvlan type driver.
The current macvlan driver already does the downstream side of
VEPA and only needs a connection to KVM et al, either using
Or's qemu packet socket interface, or using the macvtap driver
I posted.
Now Anna's patch also addresses the upstream side of VEPA, by
making it possible for the bridge code to send frames back
in the bridge code that they were received from, if that port
is marked as a hairpin mode port.
Is your suggestion to do that part also with a macvlan type driver?
I've thought about this before, and I guess that would mean
basically the same as the macvlan driver, except hashing the
source MAC address instead of the destination MAC address for
inbound frames. That way you should be able to do something
like:
Host A Host B
/- nalvcam0 -\ /- macvlan0 - 192.168.1.1
br0 -| |- ethA === ethB -|
\- nalvcam1 -/ \- macvlan1 - 192.168.1.2
Now assuming that macvlan0 and macvlan1 are in different
network namespaces or belong to different KVM guests, these
guests would be able to communicate with each other through
the bridge on host A, which can set the policy (using ebtables)
for this communication and get interface statistics on its
nalvcam interfaces. Also, instead of having the br0, Host A could
assign an IP addresses to the two nalvcam interfaces that host
B has, and use IP forwarding between the guests of host B.
Arnd <><
^ permalink raw reply
* Re: [Socketcan-users] [PATCH] CAN: make checking in can_rcv less restrictive
From: Oliver Hartkopp @ 2009-08-07 11:35 UTC (permalink / raw)
To: Rémi Denis-Courmont, Luotao Fu
Cc: netdev@vger.kernel.org, socketcan-users@lists.berlios.de,
Michael Olbrich
In-Reply-To: <200908070952.56922.remi.denis-courmont@nokia.com>
Rémi Denis-Courmont wrote:
> Moving to netdev....
>
> On Thursday 06 August 2009 19:48:23 ext Oliver Hartkopp wrote:
>> The CAN applications can rely on getting proper CAN frames with this check.
>> It was introduced some time ago together with several other sanity checks -
>> even on the TX path.
>>
>> The CAN core *only* consumes skbuffs originated from a CAN netdevice
>> (ARPHRD_CAN).
>>
>> When this BUG() triggers, someone provided a definitely broken *CAN*
>> network driver, and this needs to be fixed on that level. It is really not
>> that problematic to ensure proper CAN frames on driver level ... this
>> sanity check should not be needed to be performed by every single
>> application.
>
> AFAIK, the TUN driver can inject layer-2 frames of any type, any size and any
> content from userspace into the packet type handler. Sure enough, you need
> CAP_NET_ADMIN and r/w access to /dev/net/tun but is it sufficient to bring the
> system down?
>
The complete code section currently looks like this:
static int can_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
struct dev_rcv_lists *d;
struct can_frame *cf = (struct can_frame *)skb->data;
int matches;
if (dev->type != ARPHRD_CAN || !net_eq(dev_net(dev), &init_net)) {
kfree_skb(skb);
return 0;
}
BUG_ON(skb->len != sizeof(struct can_frame) || cf->can_dlc > 8);
(..)
So you would need to have an originating interface with ARPHRD_CAN ...
Do you think, it's still possible with the TUN driver?
@Luotao: I talked to Urs and we discussed to prepare a patch that only creates
a warning and drops the skb afterwards, as the problem is not critical for a
proper ongoing kernel operation. I think, that was you original intention:
if (!net_eq(dev_net(dev), &init_net) ||
WARN_ON(dev->type != ARPHRD_CAN) ||
WARN_ON(skb->len != sizeof(struct can_frame) || cf->can_dlc > 8)) {
kfree_skb(skb);
return NET_RX_BAD;
}
Would this be ok for you?
Regards,
Oliver
^ permalink raw reply
* Re: [Socketcan-users] [PATCH] CAN: make checking in can_rcv less restrictive
From: Luotao Fu @ 2009-08-07 11:46 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: R?mi Denis-Courmont, Luotao Fu, netdev@vger.kernel.org,
socketcan-users@lists.berlios.de, Michael Olbrich
In-Reply-To: <4A7C117E.5010005@hartkopp.net>
[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]
Hi Oliver,
On Fri, Aug 07, 2009 at 01:35:26PM +0200, Oliver Hartkopp wrote:
> R?mi Denis-Courmont wrote:
....
>
> @Luotao: I talked to Urs and we discussed to prepare a patch that only creates
> a warning and drops the skb afterwards, as the problem is not critical for a
> proper ongoing kernel operation. I think, that was you original intention:
>
> if (!net_eq(dev_net(dev), &init_net) ||
> WARN_ON(dev->type != ARPHRD_CAN) ||
> WARN_ON(skb->len != sizeof(struct can_frame) || cf->can_dlc > 8)) {
> kfree_skb(skb);
> return NET_RX_BAD;
> }
>
> Would this be ok for you?
I'm absolutely fine with this.
thx
cheers
Fu
--
Pengutronix e.K. | Dipl.-Ing. Luotao Fu |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* [PATCH] fec: fix FEC driver packet transmission breakage
From: Greg Ungerer @ 2009-08-07 3:58 UTC (permalink / raw)
To: netdev; +Cc: gerg, s.hauer
fec: fix FEC driver packet transmission breakage
Commit f0b3fbeae11a526c3d308b691684589ee37c359b breaks transmission of
packets where the skb data buffer is not memory aligned according to
FEC_ALIGNMENT. It incorrectly passes to dma_sync_single() the buffer
address directly from the skb, instead of the address calculated for
use (which may be the skb address or one of the bounce buffers).
It seems there is no use converting the cpu address of the buffer to
a physical either, since dma_map_single() expects the cpu address and
will return the dma address to use in the descriptor. So remove the use
of __pa() on the buffer address as well.
This patch is against 2.6.30-rc5. This breakage is a regression over
2.6.30, which does not have this problem.
Signed-off-by: Greg Ungerer <gerg@uclinux.org>
---
diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 0d2ab43..a32230b 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -285,6 +285,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct fec_enet_private *fep = netdev_priv(dev);
struct bufdesc *bdp;
+ void *bufaddr;
unsigned short status;
unsigned long flags;
@@ -312,7 +313,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
status &= ~BD_ENET_TX_STATS;
/* Set buffer length and buffer pointer */
- bdp->cbd_bufaddr = __pa(skb->data);
+ bufaddr = skb->data;
bdp->cbd_datlen = skb->len;
/*
@@ -320,11 +321,11 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
* 4-byte boundaries. Use bounce buffers to copy data
* and get it aligned. Ugh.
*/
- if (bdp->cbd_bufaddr & FEC_ALIGNMENT) {
+ if (((unsigned long) bufaddr) & FEC_ALIGNMENT) {
unsigned int index;
index = bdp - fep->tx_bd_base;
memcpy(fep->tx_bounce[index], (void *)skb->data, skb->len);
- bdp->cbd_bufaddr = __pa(fep->tx_bounce[index]);
+ bufaddr = fep->tx_bounce[index];
}
/* Save skb pointer */
@@ -336,7 +337,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
/* Push the data cache so the CPM does not get stale memory
* data.
*/
- bdp->cbd_bufaddr = dma_map_single(&dev->dev, skb->data,
+ bdp->cbd_bufaddr = dma_map_single(&dev->dev, bufaddr,
FEC_ENET_TX_FRSIZE, DMA_TO_DEVICE);
/* Send it on its way. Tell FEC it's ready, interrupt when done,
^ permalink raw reply related
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