From a06c8f76c2ea13793150642600ea14841e92e0aa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 1 Dec 2023 17:53:58 +0100 Subject: [PATCH] route/cls: add get/take wrappers for rtnl_act_append() rtnl_act_append() either takes ownership of the argument, or does nothing (on error). This pattern is hard to get right. In the past, there were various bugs at this place. Add two wrappers _rtnl_act_append_get() and _rtnl_act_append_take() which consistently don't take ownership of the pointer or take it. Also, in functions like rtnl_flower_append_action() only set the mask after successfully modifying the data. --- include/nl-aux-route/nl-route.h | 29 ++++++++++++++++++++++++++--- lib/route/act.c | 4 +--- lib/route/cls/basic.c | 7 +++---- lib/route/cls/flower.c | 7 +++---- lib/route/cls/mall.c | 11 +++++------ lib/route/cls/u32.c | 6 ++---- 6 files changed, 40 insertions(+), 24 deletions(-) diff --git a/include/nl-aux-route/nl-route.h b/include/nl-aux-route/nl-route.h index a0193d2b..9c85337a 100644 --- a/include/nl-aux-route/nl-route.h +++ b/include/nl-aux-route/nl-route.h @@ -5,6 +5,8 @@ #include "base/nl-base-utils.h" +#include + struct rtnl_link; void rtnl_link_put(struct rtnl_link *); #define _nl_auto_rtnl_link _nl_auto(_nl_auto_rtnl_link_fcn) @@ -40,13 +42,10 @@ void rtnl_link_af_ops_put(struct rtnl_link_af_ops *); _NL_AUTO_DEFINE_FCN_TYPED0(struct rtnl_link_af_ops *, _nl_auto_rtnl_link_af_ops_fcn, rtnl_link_af_ops_put); -struct rtnl_act; -void rtnl_act_put(struct rtnl_act *); #define _nl_auto_rtnl_act _nl_auto(_nl_auto_rtnl_act_fcn) _NL_AUTO_DEFINE_FCN_TYPED0(struct rtnl_act *, _nl_auto_rtnl_act_fcn, rtnl_act_put); -void rtnl_act_put_all(struct rtnl_act **head); #define _nl_auto_rtnl_act_all _nl_auto(_nl_auto_rtnl_act_fcn_all) _NL_AUTO_DEFINE_FCN_INDIRECT0(struct rtnl_act *, _nl_auto_rtnl_act_fcn_all, rtnl_act_put_all); @@ -64,4 +63,28 @@ void rtnl_cls_put(struct rtnl_cls *); _NL_AUTO_DEFINE_FCN_TYPED0(struct rtnl_cls *, _nl_auto_rtnl_cls_fcn, rtnl_cls_put); +/*****************************************************************************/ + +static inline int _rtnl_act_append_get(struct rtnl_act **head, + struct rtnl_act *new) +{ + int r; + + r = rtnl_act_append(head, new); + if (r >= 0) + rtnl_act_get(new); + return r; +} + +static inline int _rtnl_act_append_take(struct rtnl_act **head, + struct rtnl_act *new) +{ + int r; + + r = rtnl_act_append(head, new); + if (r < 0) + rtnl_act_put(new); + return r; +} + #endif /* __NETLINK_NL_AUX_ROUTE_NL_ROUTE_H__ */ diff --git a/lib/route/act.c b/lib/route/act.c index 2129bd00..670810fa 100644 --- a/lib/route/act.c +++ b/lib/route/act.c @@ -493,11 +493,9 @@ int rtnl_act_parse(struct rtnl_act **head, struct nlattr *tb) if (err < 0) return err; } - err = rtnl_act_append(&tmp_head, act); + err = _rtnl_act_append_take(&tmp_head, _nl_steal_pointer(&act)); if (err < 0) return err; - - _nl_steal_pointer(&act); } *head = _nl_steal_pointer(&tmp_head); diff --git a/lib/route/cls/basic.c b/lib/route/cls/basic.c index ed83ca1c..dc19256b 100644 --- a/lib/route/cls/basic.c +++ b/lib/route/cls/basic.c @@ -25,6 +25,7 @@ #include #include "tc-api.h" +#include "nl-aux-route/nl-route.h" struct rtnl_basic { @@ -223,12 +224,10 @@ int rtnl_basic_add_action(struct rtnl_cls *cls, struct rtnl_act *act) if (!(b = rtnl_tc_data(TC_CAST(cls)))) return -NLE_NOMEM; - b->b_mask |= BASIC_ATTR_ACTION; - if ((err = rtnl_act_append(&b->b_act, act))) + if ((err = _rtnl_act_append_get(&b->b_act, act)) < 0) return err; - /* In case user frees it */ - rtnl_act_get(act); + b->b_mask |= BASIC_ATTR_ACTION; return 0; } diff --git a/lib/route/cls/flower.c b/lib/route/cls/flower.c index ad86ce98..56f24c3e 100644 --- a/lib/route/cls/flower.c +++ b/lib/route/cls/flower.c @@ -15,6 +15,7 @@ #include #include "tc-api.h" +#include "nl-aux-route/nl-route.h" /** @cond SKIP */ struct rtnl_flower { @@ -817,12 +818,10 @@ int rtnl_flower_append_action(struct rtnl_cls *cls, struct rtnl_act *act) if (!(f = rtnl_tc_data(TC_CAST(cls)))) return -NLE_NOMEM; - f->cf_mask |= FLOWER_ATTR_ACTION; - - if ((err = rtnl_act_append(&f->cf_act, act)) < 0) + if ((err = _rtnl_act_append_get(&f->cf_act, act)) < 0) return err; - rtnl_act_get(act); + f->cf_mask |= FLOWER_ATTR_ACTION; return 0; } diff --git a/lib/route/cls/mall.c b/lib/route/cls/mall.c index 04ec58a2..030eaa81 100644 --- a/lib/route/cls/mall.c +++ b/lib/route/cls/mall.c @@ -20,6 +20,7 @@ #include #include "tc-api.h" +#include "nl-aux-route/nl-route.h" struct rtnl_mall { uint32_t m_classid; @@ -107,12 +108,10 @@ int rtnl_mall_append_action(struct rtnl_cls *cls, struct rtnl_act *act) if (!(mall = rtnl_tc_data(TC_CAST(cls)))) return -NLE_NOMEM; - mall->m_mask |= MALL_ATTR_ACTION; - err = rtnl_act_append(&mall->m_act, act); - if (err < 0) - return err; + if ((err = _rtnl_act_append_get(&mall->m_act, act)) < 0) + return err; - rtnl_act_get(act); + mall->m_mask |= MALL_ATTR_ACTION; return 0; } @@ -245,7 +244,7 @@ static int mall_clone(void *_dst, void *_src) if (!new) return -NLE_NOMEM; - err = rtnl_act_append(&dst->m_act, new); + err = _rtnl_act_append_take(&dst->m_act, new); if (err < 0) return err; diff --git a/lib/route/cls/u32.c b/lib/route/cls/u32.c index ab8b9774..52f6e31c 100644 --- a/lib/route/cls/u32.c +++ b/lib/route/cls/u32.c @@ -612,12 +612,10 @@ int rtnl_u32_add_action(struct rtnl_cls *cls, struct rtnl_act *act) if (!(u = rtnl_tc_data(TC_CAST(cls)))) return -NLE_NOMEM; - u->cu_mask |= U32_ATTR_ACTION; - if ((err = rtnl_act_append(&u->cu_act, act))) + if ((err = _rtnl_act_append_get(&u->cu_act, act)) < 0) return err; - /* In case user frees it */ - rtnl_act_get(act); + u->cu_mask |= U32_ATTR_ACTION; return 0; }