On Fri, May 12, 2023 at 11:25:02AM +0200, Frode Nordahl wrote: > 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?
Yes, that would be what I would do too. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
