[ovs-dev] [bugfixes 3/3] datapath: Dump flow actions only if there is room.

Jesse Gross jesse at nicira.com
Tue Feb 1 02:44:16 UTC 2011

On Mon, Jan 31, 2011 at 4:48 PM, Ben Pfaff <blp at nicira.com> wrote:
> Expanding an skbuff in a netlink dump handler doesn't work well.  First,
> we weren't updating the truesize of the skb or the allocation within the
> socket that netlink_dump() had put the skb in.  Even when I fixed that, I
> still got mysterious crashes and reboots.  I don't know why these happened.
> This commit fixes the problem (in my tests, anyway) by avoiding expanding
> the reply skbuff to fill in the actions.  Instead, in such a case the
> userspace client has to do a separate "get" action to get the actions.
> This commit also updates userspace to do this automatically for dumps in
> the cases where the caller cares (only "ovs-dpctl dump-flows" currently
> cares).
> Signed-off-by: Ben Pfaff <blp at nicira.com>

This looks fine to me.  However, I think that I see the problem in the
original code that is causing this error.

Currently, if the actions do not fit in the skb we expand it if there
are no other flows.  After expansion, if necessary, we assume that we
have sufficient space in the packet since we've already checked and
just blindly copy data.  However, if the packet was not originally
empty, we do not expand but also do not abort: we blindly copy data.
This explains why we were having problems when there were only small
numbers of actions since we only needed the number of flows to exceed
the total size and for the action list to fall on the boundary.  It
also explains why we were getting errors about freeing paged data:
skb_shared_info is located at the end of the linear data area and
contains the information about pages, which we were overwriting.

If we do continue to reallocate the buffer as necessary, it seems that
we might need to also check the return code of
odp_flow_cmd_fill_info() more carefully: a memory allocation failure
will return a truncated list with no indication of error.  On the
other hand, it doesn't seem that netlink_dump() is all that careful
about errors (nothing checks it's error code for allocation failure
for example).

More information about the dev mailing list