On Tue, Oct 08, 2013 at 04:48:04PM -0700, Jarno Rajahalme wrote:
> On Oct 8, 2013, at 2:56 PM, b...@nicira.com wrote:
> > Since this adds the 'zeros' field back to struct flow, it needs to add
> > code to actually zero that field in a few places. Take a look at the
> > corresponding code that was removed in commit b0a17866c3145 (Remove
> > mpls_depth field from flow).
> >
>
> Does it really matter what the values of "zeros" are?
It matters to flow_hash() and flow_compare_3way(). There are other
ways to define these functions that avoid the problem, but overall I
have not found a better way than zeroing the pad bytes.
> Maybe it should be renamed as "pad" instead.. After all, we do not
> seem to care about the implicit padding of 4 bytes in struct
> flow_tnl.
We need to make sure that those are zeroed too. Perhaps we've missed
some places...
> How about removing "zeros" and let the compiler care about
> the padding, assuming there is a clean fix for the struct size test
> considering the size difference on 32/64 bit systems?
Well, that would be awesome, all we need is fixes for hashing and
comparison.
> > In mf_random_value(), I think the mask is reversed, that is, that the
> > ~ should be removed.
>
> Will fix.
>
> It seems MFF_IPV6_LABEL has the same problem.
Yes, good spotting, thank you. Will you fix it, or shall I?
> >
> > In parse_odp_key_mask_attr() below, I don't understand why there is a
> > test for tcp_flags != 0:
> > + uint16_t tcp_flags, tcp_flags_mask;
> > + int n = -1;
> > +
> > + if (mask && sscanf(s, "tcp_flags(%"SCNi16"/%"SCNi16")%n",
> > + &tcp_flags, &tcp_flags_mask, &n) > 0 && n > 0) {
> > + if (tcp_flags != 0) {
> > + nl_msg_put_be16(key, OVS_KEY_ATTR_TCP_FLAGS,
> > htons(tcp_flags));
> > + }
> > + nl_msg_put_be16(mask, OVS_KEY_ATTR_TCP_FLAGS,
> > htons(tcp_flags_mask));
> > + return n;
> >
>
> I guess there is no reason, other than that I used the same pattern
> in odp_flow_key_from_flow__(), where it actually makes a difference
> (avoid creating keys that an older kernel cannot understand). Here
> we are setting the mask anyway, so this looks weird. However, this
> should make no functional difference, as missing key attars are
> generally treated as zeros. Will remove to check to avoid confusion.
Thanks.
> > In odp_flow_key_from_flow__(), I think that we need some new logic for
> > fragment handling. The TCP source and dest port are always in the
> > first fragment, because they are in the first 8 bytes, but the TCP
> > flags start at offset 12, so they cannot be guaranteed to be in the
> > first fragment, though of course most of the time they will be.
>
> This due to the minimum Ethernet frame size cutting in between?
The issue is not the minimum Ethernet frame size but the minimum IP
fragment size. IP fragments are 8-byte aligned and a multiple of 8
bytes in length. One way to implement a simple firewall that handles
fragments properly is to pass all "later" fragments (those with an
offset greater than 0) and drop "first" fragments (those with an
offset of 0) for disallowed IP/port combinations. Then one cannot
usefully evade the firewall: the "later" fragments pass the firewall,
but the destination host cannot reassemble them into full IP datagrams
and will eventually drop them because it never gets the first
fragment.
One might naturally want to include TCP flags in such a check, but
those would not necessarily be in the first fragment.
> My understanding is that TCP tries to use MTU-sized packets, and
> no-one has 64 byte MTU in their networks, so the only reasonable
> case where the TCP header would be split would be an attack trying
> to circumvent TCP flag filtering. To be on the safe side, I think we
> should drop TCP packets where the whole TCP header is not in the
> first fragment.
I agree that that is a reasonable policy, but I am not sure that it
makes sense to apply it at a low level. The kernel module should not
unconditionally drop such packets, because one should be able to
implement a dumb switch or hub with the kernel module. Userspace
should probably not unconditionally drop all such packets either,
because (similarly) one should be able to implement a dumb switch or
hub with OpenFlow. A controller could reasonably implement such a
policy, but it would need to be able to distinguish those packets from
others via OpenFlow. We have extra bits, so one way to do that would
be to set, say, bit 12 (and leave the rest zero) if the TCP flags are
unavailable due to fragmentation. Other approaches are possible too.
> Wouldn't this be even worse problem for IPv6?
Yes.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
