[ovs-dev] [patch v9 06/11] Userspace datapath: Add fragmentation handling.

Darrell Ball dlu998 at gmail.com
Wed Dec 12 08:45:41 UTC 2018


I'll respond to the individual comments soon, although some comments are
answered by
subsequent patches.
I did notice a problem that I will fix and make adjustments for, in that
the ipf context should
have been per datapath.

Thanks Darrell

On Tue, Dec 11, 2018 at 8:15 AM Ben Pfaff <blp at ovn.org> wrote:

> On Mon, Nov 19, 2018 at 11:09:25AM -0800, Darrell Ball wrote:
> > Fragmentation handling is added for supporting conntrack.
> > Both v4 and v6 are supported.
> >
> > After discussion with several people, I decided to not store
> > configuration state in the database to be more consistent with
> > the kernel in future, similarity with other conntrack configuration
> > which will not be in the database as well and overall simplicity.
> > Accordingly, fragmentation handling is enabled by default.
> >
> > This patch enables fragmentation tests for the userspace datapath.
> >
> > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
>
> Thanks for implementing this.
>
> This could use more comments, especially on the data structures and
> file-level variables and at the level of an individual function.
>
> Please don't invent yet another mutex.
>
> ipf_print_reass_packet() seems like it could use ds_put_hex_dump() and
> thereby be more readable (I don't really want to read 182 hex digits in
> a row without any white space).
>
> Why 91 bytes in ipf_print_reass_packet()?
>
> All the counters seem to be write-only.
>
> struct ipf_addr seems odd to me.  It has both aligned and unaligned
> versions of addresses, which means that the overall struct needs to be
> aligned, and it's a union nested in a struct instead of just a union.
> ipf_addr_hash_add() implies that all the bytes in the struct need to be
> initialized even if only some of them are used.
>
> ipf_list_key_hash() seems to be at risk of hashing trailing padding at
> the end of struct ipf_list_key.
>
> Does ipf_list_complete() run one past the end of the array?  Naively, it
> seems like it might.
>
> I found ipf_sort() a little hard to read mostly due to the long variable
> names.  Here's an alternate form that you can accept or reject as you
> like (I have not tested it):
>
> static void
> ipf_sort(struct ipf_frag *frags, size_t last_idx)
>     OVS_REQUIRES(ipf_lock)
> {
>     for (int i = 1; i <= last_idx; i++) {
>         struct ipf_frag ipf_frag = frags[i];
>         int j = i - 1;
>         while (j >= 0 && frags[j].start_data_byte >
> ipf_frag.start_data_byte) {
>             frags[j + 1] = frags[j];
>             j--;
>         }
>         frags[j + 1] = ipf_frag;
>     }
> }
>
> ipf_reassemble_v4_frags() and ipf_reassemble_v6_frags() know the final
> length of the packet they're assembling, but it doesn't look to me like
> they preallocate the space for it.
>
> ipf_reassemble_v6_frags() and ipf_reassemble_v6_frags() calculate the
> length of the L3 header manually and skip past it.  Couldn't they just
> use the L4 header pointer?
>
> The ipf_list_key_cmp() interface is a little confusing because the
> return value convention and the name is a little like a strcmp()ish
> function, but it doesn't use a -1/+1 return value.  I'd rename it to
> ipf_list_key_equal()s, change the return type to "bool", and make true
> mean equal, false mean unequal.
>
> Processing a fragment is O(n) in the number of existing fragments due to
> the dup check.  I don't know whether this is important.
>
> It looks like packet duplication can cause a fraglist to be marked
> invalid.  I wonder whether this is good behavior.
>
> It seems like we could estimate the number of fragments needed by
> dividing the total size by the size of the first fragment received.
>
> Do we need xzalloc() for frag_list?  It seems like we're going to
> initialize each element as needed anyway.
>
> I think there's a race if either v4 or v6 is ever disabled, because the
> code checks whether fragment reassembly is enabled twice, once at a high
> level and once again in ipf_handle_frag().  The latter function
> assert-fails if reassembly is disabled, which seems like it could be a
> problem.
>
> At the same time, I don't see any way to ever disable fragment
> reassembly.  The code appears to enable it by default and not provide
> any way to disable it.
>
> Is it common practice to enforce a minimum size for fragments?
>
> Maybe it would be a little easier to make the counters a two-dimensional
> array: atomic_uint64_t ipf_counters[2][N_COUNTERS], where one dimension
> is IPv4/IPv6 and the other is the particular counter.  Then ipf_count()
> would become trivial.  Just a thought though.
>
> Thanks, and I'll look forward to the next version.
>


More information about the dev mailing list