On Fri, Nov 11, 2011 at 05:21:52PM -0800, Jesse Gross wrote:
> On Fri, Nov 11, 2011 at 5:18 PM, Ben Pfaff <b...@nicira.com> wrote:
> > On Fri, Nov 11, 2011 at 03:50:56PM -0800, Ben Pfaff wrote:
> >> On Fri, Nov 11, 2011 at 03:48:25PM -0800, Jesse Gross wrote:
> >> > On Fri, Nov 11, 2011 at 3:43 PM, Ben Pfaff <b...@nicira.com> wrote:
> >> > > On Fri, Nov 11, 2011 at 03:38:51PM -0800, Jesse Gross wrote:
> >> > >> On Fri, Nov 11, 2011 at 3:13 PM, Ben Pfaff <b...@nicira.com> wrote:
> >> > >> > I guess that this will need reconsideration after the "encap"
> >> > >> > series,
> >> > >> > so I guess there's no point in a detailed response right now--let me
> >> > >> > know if you disagree.
> >> > >>
> >> > >> I agree that most of this patch has been overcome by events. ??I think
> >> > >> there are a few useful things here that aren't addressed by the latest
> >> > >> patch series though:
> >> > >> ??* Packets with invalid vlan headers shouldn't be dropped (maybe this
> >> > >> is another rule, that invalid protocol headers should never drop the
> >> > >> packet?).
> >> > >> ??* I think that if the EtherType is 0x8100 then we should expect a
> >> > >> vlan attribute which may be an empty one, similar to other L3
> >> > >> protocols.
> >> > >
> >> > > OK, do you want to address that or do you want me to do it?
> >> >
> >> > Could you do it while I review this series?
> >>
> >> OK.
> >
> > This is really preliminary. ??It compiles and passes "make check", I
> > won't vouch for anything else, hence no signed-off-by yet. ??It applies
> > after the "encap" series.
> >
> > Commit message is the same one you had, I haven't updated it at all.
>
> I haven't looked at the code at all but we should also add to the
> rules the principles that this patch implements (don't drop packets
> for broken tags; if the previous layer indicates that the attribute
> should be present then it should be, even if empty).
How about this:
Handling malformed packets
--------------------------
Don't drop packets in the kernel for malformed protocols headers, bad
checksums, etc. This would prevent userspace from implementing a
simple Ethernet switch that forwards every packet.
Instead, in such a case, include an attribute with "empty" content.
It doesn't matter if the empty content could be valid protocol values,
as long as those values are rarely seen in practice, because userspace
can always forward all packets with those values to userspace and
handle them individually.
For example, consider a packet that contains an IP header that
indicates protocol 6 for TCP, but which is truncated just after the IP
header, so that the TCP header is missing. The flow key for this
packet would include a tcp attribute with all-zero src and dst, like
this:
eth(...), eth_type(0x0800), ip(proto=6, ...), tcp(src=0, dst=0)
As another example, consider a packet with an Ethernet type of 0x8100,
indicating that a VLAN TCI should follow, but which is truncated just
after the Ethernet type. The flow key for this packet would include
an all-zero-bits vlan and an empty encap attribute, like this:
eth(...), eth_type(0x8100), vlan(0), encap()
Unlike a TCP packet with source and destination ports 0, an
all-zero-bits VLAN TCI is not that rare, so the CFI bit (aka
VLAN_TAG_PRESENT inside the kernel) is ordinarily set in a vlan
attribute expressly to allow this situation to be distinguished.
Thus, the flow key in this second example unambiguously indicates a
missing or malformed VLAN TCI.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
