Re: [PATCH v4 nf-next 2/2] netfilter: nf_flow_table_core: teardown direct xmit when destination changed

[Florian Westphal <fw@strlen.de>]

Eric Woudstra <ericwouds@gmail.com> wrote:
> +static void nf_flow_table_do_cleanup_addr(struct nf_flowtable *flow_table,
> +					  struct flow_offload *flow, void *data)
> +{
> +	struct flow_cleanup_data *cud = data;
> +
> +	if ((flow->tuplehash[0].tuple.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
> +	     flow->tuplehash[0].tuple.out.ifidx == cud->ifindex &&
> +	     flow->tuplehash[0].tuple.out.bridge_vid == cud->vid &&
> +	     ether_addr_equal(flow->tuplehash[0].tuple.out.h_dest, cud->addr)) ||
> +	    (flow->tuplehash[1].tuple.xmit_type == FLOW_OFFLOAD_XMIT_DIRECT &&
> +	     flow->tuplehash[1].tuple.out.ifidx == cud->ifindex &&
> +	     flow->tuplehash[1].tuple.out.bridge_vid == cud->vid &&
> +	     ether_addr_equal(flow->tuplehash[1].tuple.out.h_dest, cud->addr))) {

I think it would be better to have a helper for this, so
it boils down to:
if (__nf_flow_table_do_cleanup_addr(flow->tuplehash[0]) ||
    __nf_flow_table_do_cleanup_addr(flow->tuplehash[1]))

(thats assuming we can go forward with the full walk.)

> +static int nf_flow_table_switchdev_event(struct notifier_block *unused,
> +					 unsigned long event, void *ptr)
> +{
> +	struct flow_switchdev_event_work *switchdev_work;
> +	struct switchdev_notifier_fdb_info *fdb_info;
> +
> +	if (event != SWITCHDEV_FDB_DEL_TO_DEVICE)
> +		return NOTIFY_DONE;
> +
> +	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
> +	if (WARN_ON(!switchdev_work))
> +		return NOTIFY_BAD;

No WARN_ON here.  GFP_ATOMIC can fail, which then gives a splat.
But there is nothing that could be done about it for either reporter
or developer.

So, how much of a problem is this?
If its fine to ignore the notification, then remove the WARN_ON.
If its not ok, then you have to explore alternatives that do not depend
on successful allocation.

Can the invalided output port be detected from packet path similar to
how stale dsts get handled?