| Message ID | 20190411215419.5417-1-vladbu@mellanox.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: sched: flower: fix filter net reference counting | expand |
On Fri, Apr 12, 2019 at 12:54:19AM +0300, Vlad Buslov wrote: > Fix net reference counting in fl_change() and remove redundant call to > tcf_exts_get_net() from __fl_delete(). __fl_put() already tries to get net > before releasing exts and deallocating a filter, so this code caused flower > classifier to obtain net twice per filter that is being deleted. > > Implementation of __fl_delete() called tcf_exts_get_net() to pass its > result as 'async' flag to fl_mask_put(). However, 'async' flag is redundant > and only complicates fl_mask_put() implementation. This functionality seems > to be copied from filter cleanup code, where it was added by Cong with > following explanation: > > This patchset tries to fix the race between call_rcu() and > cleanup_net() again. Without holding the netns refcnt the > tc_action_net_exit() in netns workqueue could be called before > filter destroy works in tc filter workqueue. This patchset > moves the netns refcnt from tc actions to tcf_exts, without > breaking per-netns tc actions. > > This doesn't apply to flower mask, which doesn't call any tc action code > during cleanup. Simplify fl_mask_put() by removing the flag parameter and > always use tcf_queue_work() to free mask objects. > > Fixes: 061775583e35 ("net: sched: flower: introduce reference counting for filters") > Fixes: 1f17f7742eeb ("net: sched: flower: insert filter to ht before offloading it to hw") > Fixes: 05cd271fd61a ("cls_flower: Support multiple masks per priority") > Reported-by: Ido Schimmel <idosch@mellanox.com> > Signed-off-by: Vlad Buslov <vladbu@mellanox.com> I see the patch was already applied, but FWIW: Tested-by: Ido Schimmel <idosch@mellanox.com>
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 2763176e369c..8dd2fbce1b37 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -336,8 +336,7 @@ static void fl_mask_free_work(struct work_struct *work) fl_mask_free(mask); } -static bool fl_mask_put(struct cls_fl_head *head, struct fl_flow_mask *mask, - bool async) +static bool fl_mask_put(struct cls_fl_head *head, struct fl_flow_mask *mask) { if (!refcount_dec_and_test(&mask->refcnt)) return false; @@ -348,10 +347,7 @@ static bool fl_mask_put(struct cls_fl_head *head, struct fl_flow_mask *mask, list_del_rcu(&mask->list); spin_unlock(&head->masks_lock); - if (async) - tcf_queue_work(&mask->rwork, fl_mask_free_work); - else - fl_mask_free(mask); + tcf_queue_work(&mask->rwork, fl_mask_free_work); return true; } @@ -538,7 +534,6 @@ static int __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f, struct netlink_ext_ack *extack) { struct cls_fl_head *head = fl_head_dereference(tp); - bool async = tcf_exts_get_net(&f->exts); *last = false; @@ -555,7 +550,7 @@ static int __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f, list_del_rcu(&f->list); spin_unlock(&tp->lock); - *last = fl_mask_put(head, f->mask, async); + *last = fl_mask_put(head, f->mask); if (!tc_skip_hw(f->flags)) fl_hw_destroy_filter(tp, f, rtnl_held, extack); tcf_unbind_filter(tp, &f->res); @@ -1605,11 +1600,10 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, spin_unlock(&tp->lock); - fl_mask_put(head, fold->mask, true); + fl_mask_put(head, fold->mask); if (!tc_skip_hw(fold->flags)) fl_hw_destroy_filter(tp, fold, rtnl_held, NULL); tcf_unbind_filter(tp, &fold->res); - tcf_exts_get_net(&fold->exts); /* Caller holds reference to fold, so refcnt is always > 0 * after this. */ @@ -1657,8 +1651,9 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, rhashtable_remove_fast(&fnew->mask->ht, &fnew->ht_node, fnew->mask->filter_ht_params); errout_mask: - fl_mask_put(head, fnew->mask, true); + fl_mask_put(head, fnew->mask); errout: + tcf_exts_get_net(&fnew->exts); tcf_queue_work(&fnew->rwork, fl_destroy_filter_work); errout_tb: kfree(tb);
Fix net reference counting in fl_change() and remove redundant call to tcf_exts_get_net() from __fl_delete(). __fl_put() already tries to get net before releasing exts and deallocating a filter, so this code caused flower classifier to obtain net twice per filter that is being deleted. Implementation of __fl_delete() called tcf_exts_get_net() to pass its result as 'async' flag to fl_mask_put(). However, 'async' flag is redundant and only complicates fl_mask_put() implementation. This functionality seems to be copied from filter cleanup code, where it was added by Cong with following explanation: This patchset tries to fix the race between call_rcu() and cleanup_net() again. Without holding the netns refcnt the tc_action_net_exit() in netns workqueue could be called before filter destroy works in tc filter workqueue. This patchset moves the netns refcnt from tc actions to tcf_exts, without breaking per-netns tc actions. This doesn't apply to flower mask, which doesn't call any tc action code during cleanup. Simplify fl_mask_put() by removing the flag parameter and always use tcf_queue_work() to free mask objects. Fixes: 061775583e35 ("net: sched: flower: introduce reference counting for filters") Fixes: 1f17f7742eeb ("net: sched: flower: insert filter to ht before offloading it to hw") Fixes: 05cd271fd61a ("cls_flower: Support multiple masks per priority") Reported-by: Ido Schimmel <idosch@mellanox.com> Signed-off-by: Vlad Buslov <vladbu@mellanox.com> --- net/sched/cls_flower.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)