On Fri, May 12, 2023 at 10:35 AM Simon Horman <simon.hor...@corigine.com> wrote:
>
> On Thu, May 11, 2023 at 12:03:50PM +0200, Frode Nordahl wrote:
> > The tc module combines the use of the `tc_transact` helper
> > function for communication with the in-kernel tc infrastructure
> > with assertions on the reply data by `ofpbuf_at_assert` on the
> > received data prior to further processing.
> >
> > `tc_transact` in turn calls `nl_transact`, which via
> > `nl_transact_multiple__` ultimately calls and handles return
> > value from `recvmsg`. On error a check for EAGAIN is performed
> > and a consequence of this condition is effectively to provide a
> > non-error (0) result and an empty reply buffer.
> >
> > Before this change, the `tc_transact` and, consumers of it, were
> > unaware of this condition. The use of assertions on the reply
> > buffer can as such lead to a fatal crash of OVS.
> >
> > To be fair, the behavior of `nl_transact` when handling an EAGAIN
> > return is currently not documented, so this change also adds that
> > documentation.
> >
> > While fixing the problem, it led me to find potential problems
> > with several of the one-time initialization functions in
> > the netdev-offload-tc module. Before this patch they would
> > happily accept temporary failure as permanent one, with
> > consequences for the remaining lifetime of the process.
> >
> > For the record, the symptom of the crash is this in the log:
> > EMER|../include/openvswitch/ofpbuf.h:194: assertion offset + size <=
> > b->size failed in ofpbuf_at_assert()
> >
> > And an excerpt of the backtrace looks like this:
> > 0x0000561dac1396d1 in ofpbuf_at_assert (b=0x7fb650005d20, b=0x7fb650005d20,
> > offset=16, size=20) at ../include/openvswitch/ofpbuf.h:194
> > tc_replace_flower (id=<optimized out>, flower=<optimized out>) at
> > ../lib/tc.c:3223
> > 0x0000561dac128155 in netdev_tc_flow_put (netdev=0x561dacf91840,
> > match=<optimized out>, actions=<optimized out>, actions_len=<optimized out>,
> > ufid=<optimized out>, info=<optimized out>, stats=<optimized out>) at
> > ../lib/netdev-offload-tc.c:2096
> > 0x0000561dac117541 in netdev_flow_put (stats=<optimized out>,
> > info=0x7fb65b7ba780, ufid=<optimized out>, act_len=<optimized out>,
> > actions=<optimized out>,
> > match=0x7fb65b7ba980, netdev=0x561dacf91840) at ../lib/netdev-offload.c:257
> > parse_flow_put (put=0x7fb65b7bcc50, dpif=0x561dad0ad550) at
> > ../lib/dpif-netlink.c:2297
> > try_send_to_netdev (op=0x7fb65b7bcc48, dpif=0x561dad0ad550) at
> > ../lib/dpif-netlink.c:2384
> >
> > Reported-At: https://launchpad.net/bugs/2018500
> > Fixes f98e418fbdb6 (tc: Add tc flower functions)
> > Fixes 407556ac6c90 (netlink-socket: Avoid forcing a reply for final message
> > in a transaction.)
> > Signed-off-by: Frode Nordahl <frode.nord...@canonical.com>
>
> Hi Frode,
>
> thanks for the patch, a nettlesome problem to be sure.
Thank you for taking the time to review!
> ...
>
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 36620199e..a6065bf69 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -5817,8 +5817,11 @@ tc_get_policer_action(uint32_t index, struct
> > ofputil_meter_stats *stats)
> >
> > error = tc_transact(&request, &replyp);
> > if (error) {
> > - VLOG_ERR_RL(&rl, "Failed to dump police action (index: %u),
> > err=%d",
> > - index, error);
> > + VLOG_RL(&rl,
> > + (error == EAGAIN) ? VLL_DBG : VLL_ERR,
> > + "Failed to dump police action (index: %u), err=%d",
> > + index,
> > + error);
>
> nit: could we economise the number of lines?
Sure, I will compact these.
> VLOG_RL(&rl, (error == EAGAIN) ? VLL_DBG : VLL_ERR,
> "Failed to dump police action (index: %u), err=%d",
> index, error);
>
> > return error;
> > }
> >
>
> ...
>
> > @@ -2594,7 +2595,9 @@ probe_multi_mask_per_prio(int ifindex)
> > memset(&flower.mask.dst_mac, 0xff, sizeof flower.mask.dst_mac);
> >
> > id1 = tc_make_tcf_id(ifindex, block_id, prio, TC_INGRESS);
> > - error = tc_replace_flower(&id1, &flower);
> > + do {
> > + error = tc_replace_flower(&id1, &flower);
>
> Perhaps there is a reason why this can't arise,
> but I'm concerned that this loop could spin forever.
I understand the concern, my rationale is that if we make the wrong decision
here because of a temporary error, it will have consequences for the rest of
the execution time of the process/thread, which may be a source of
inconsistent behavior. But we absolutely do not want to hang forever either.
I'll revisit this and see if there is a way to retry the
initialization steps for a
finite amount of attempts/time and move on with a warning/error logged
after that.
Does that sound reasonable?
--
Frode Nordahl
> > + } while (error == EAGAIN);
> > if (error) {
> > goto out;
> > }
>
> ...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
