On Fri, Dec 2, 2016 at 1:42 AM, Jiri Benc <jb...@redhat.com> wrote: > On Thu, 1 Dec 2016 12:31:09 -0800, Pravin Shelar wrote: >> On Wed, Nov 30, 2016 at 6:30 AM, Jiri Benc <jb...@redhat.com> wrote: >> > I'm not opposed to changing this but I'm afraid it needs much deeper >> > review. Because with this in place, no core kernel functions that >> > depend on skb->protocol may be called from within openvswitch. >> > >> Can you give specific example where it does not work? > > I can't, I haven't reviewed the usage. I'm just saying that the stack > does not expect skb->protocol being ETH_P_8021Q for e.g. IPv4 packets. > It may not be relevant for the calls used by openvswitch but we should > be sure about that. Especially defragmentation and conntrack is worth > looking at. > > Again, I'm not saying this is wrong nor that there is an actual > problem. I'm just pointing out that openvswitch has different > expectations about skb wrt. vlans than the rest of the kernel and we > should be reasonably sure the behavior is correct when passing between > the two. > I agree that conntrack does not expect skb-protocol to be vlan protocol. We could accelerate vlan if there is vlan header in packet itself. That would make the packet consistent across upcalls.
>> skb-protocol value is set by the caller, so it should not be >> arbitrary. is it missing in any case? > > It's not set exactly by the caller, because that's what this patch is > removing. It is set by whoever handed over the packet to openvswitch. > The point is we don't know *what* it is set to. It may as well be > ETH_P_8021Q, breaking the conditions here. It should not happen in > practice but still, it seems weird to depend on the fact that the > packet coming to ovs has never skb->protocol equal to ETH_P_8021Q nor > ETH_P_8021AD. > We are kind of dependent on this atleast for L3 packets injected back by vswitchd. For rest of entry points I think we have to trust the networking stack would set skb-protocol to correct value. If that is not true in some case, it is bug and we will need to fix it.
