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 <[email protected]>
Hi Frode,
thanks for the patch, a nettlesome problem to be sure.
...
> 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?
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.
> + } while (error == EAGAIN);
> if (error) {
> goto out;
> }
...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
