On Tue, 1 Dec 2020 09:55:37 +0200 Vlad Buslov wrote:
> On Tue 01 Dec 2020 at 04:52, Jakub Kicinski <k...@kernel.org> wrote:
> > On Fri, 27 Nov 2020 17:12:05 +0200 Vlad Buslov wrote:
> >> @@ -2262,7 +2260,7 @@ static int tc_del_tfilter(struct sk_buff *skb,
> >> struct nlmsghdr *n,
> >>
> >> if (prio == 0) {
> >> tfilter_notify_chain(net, skb, block, q, parent, n,
> >> - chain, RTM_DELTFILTER, rtnl_held);
> >> + chain, RTM_DELTFILTER);
> >> tcf_chain_flush(chain, rtnl_held);
> >> err = 0;
> >> goto errout;
> >
> > Hum. This looks off.
>
> Hi Jakub,
>
> Prio==0 means user requests to flush whole chain. In such case rtnl lock
> is obtained earlier in tc_del_tfilter():
>
> /* Take rtnl mutex if flushing whole chain, block is shared (no qdisc
> * found), qdisc is not unlocked, classifier type is not specified,
> * classifier is not unlocked.
> */
> if (!prio ||
> (q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) ||
> !tcf_proto_is_unlocked(name)) {
> rtnl_held = true;
> rtnl_lock();
> }
>
Makes sense, although seems a little fragile. Why not put a true in
there, in that case?
Do you have a larger plan here? The motivation seems a little unclear
if I'm completely honest. Are you dropping the rtnl_held from all callers
of __tcf_get_next_proto() just to save the extra argument / typing?
That's nice but there's also value in the API being consistent.
