| Message ID | 20180905190414.5477-2-viro@ZenIV.linux.org.uk |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/7] fix hnode refcounting | expand |
On 2018-09-05 3:04 p.m., Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > ... and disallow deleting or linking to such > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Same comment as other one in regards to subject Since the flag space is coming from htnode which is exposed via uapi it makes sense to keep this one here because it is for private use; but a comment in include/uapi/linux/pkt_cls.h that this flag or maybe a set of bits is reserved for internal use. Otherwise: Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal
On 2018-09-06 6:28 a.m., Jamal Hadi Salim wrote: > On 2018-09-05 3:04 p.m., Al Viro wrote: >> From: Al Viro <viro@zeniv.linux.org.uk> >> >> ... and disallow deleting or linking to such >> >> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > Same comment as other one in regards to subject > > Since the flag space is coming from htnode which is > exposed via uapi it makes sense to keep this one here > because it is for private use; but a comment in > include/uapi/linux/pkt_cls.h that this flag or > maybe a set of bits is reserved for internal use. > Otherwise: > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> Sorry, additional comment: It makes sense to reject user space attempt to set TCA_CLS_FLAGS_U32_ROOT So my suggestion is to update tc_flags_valid() to check for this. cheers, jamal
And a bunch of indentations...
cheers,
jamal
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 6d45ec4c218c..cb3bee12af78 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -485,7 +485,8 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h,
struct tc_cls_u32_offload cls_u32 = {};
tc_cls_common_offload_init(&cls_u32.common, tp,
- h->flags & ~TCA_CLS_FLAGS_U32_ROOT, extack);
+ h->flags & ~TCA_CLS_FLAGS_U32_ROOT,
+ extack);
cls_u32.command = TC_CLSU32_DELETE_HNODE;
cls_u32.hnode.divisor = h->divisor;
cls_u32.hnode.handle = h->handle;
@@ -1215,7 +1216,7 @@ static int u32_reoffload_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
int err;
tc_cls_common_offload_init(&cls_u32.common, tp,
- ht->flags & ~TCA_CLS_FLAGS_U32_ROOT, extack);
+ ht->flags & ~TCA_CLS_FLAGS_U32_ROOT, extack);
cls_u32.command = add ? TC_CLSU32_NEW_HNODE : TC_CLSU32_DELETE_HNODE;
cls_u32.hnode.divisor = ht->divisor;
cls_u32.hnode.handle = ht->handle;
On Thu, Sep 06, 2018 at 06:34:00AM -0400, Jamal Hadi Salim wrote: > On 2018-09-06 6:28 a.m., Jamal Hadi Salim wrote: > > On 2018-09-05 3:04 p.m., Al Viro wrote: > > > From: Al Viro <viro@zeniv.linux.org.uk> > > > > > > ... and disallow deleting or linking to such > > > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > > > Same comment as other one in regards to subject > > > > Since the flag space is coming from htnode which is > > exposed via uapi it makes sense to keep this one here > > because it is for private use; but a comment in > > include/uapi/linux/pkt_cls.h that this flag or > > maybe a set of bits is reserved for internal use. > > Otherwise: > > > > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> > > > Sorry, additional comment: > It makes sense to reject user space attempt to > set TCA_CLS_FLAGS_U32_ROOT Point, and that one is IMO enough to give up on using ->flags for that. How about simply diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index 3f985f29ef30..d14048e38b5c 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -84,6 +84,7 @@ struct tc_u_hnode { int refcnt; unsigned int divisor; struct idr handle_idr; + bool is_root; struct rcu_head rcu; u32 flags; /* The 'ht' field MUST be the last field in structure to allow for @@ -377,6 +378,7 @@ static int u32_init(struct tcf_proto *tp) root_ht->refcnt++; root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000; root_ht->prio = tp->prio; + root_ht->is_root = true; idr_init(&root_ht->handle_idr); if (tp_c == NULL) { @@ -693,7 +695,7 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last, goto out; } - if (root_ht == ht) { + if (ht->is_root) { NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node"); return -EINVAL; } @@ -795,6 +797,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, NL_SET_ERR_MSG_MOD(extack, "Link hash table not found"); return -EINVAL; } + if (ht_down->is_root) { + NL_SET_ERR_MSG_MOD(extack, "Not linking to root node"); + return -EINVAL; + } ht_down->refcnt++; }
On 2018-09-06 6:59 a.m., Al Viro wrote: > On Thu, Sep 06, 2018 at 06:34:00AM -0400, Jamal Hadi Salim wrote: >> On 2018-09-06 6:28 a.m., Jamal Hadi Salim wrote: [..] > Point, and that one is IMO enough to give up on using ->flags for > that. How about simply > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > index 3f985f29ef30..d14048e38b5c 100644 > --- a/net/sched/cls_u32.c > +++ b/net/sched/cls_u32.c > @@ -84,6 +84,7 @@ struct tc_u_hnode { > int refcnt; > unsigned int divisor; > struct idr handle_idr; > + bool is_root; > struct rcu_head rcu; > u32 flags; > /* The 'ht' field MUST be the last field in structure to allow for > @@ -377,6 +378,7 @@ static int u32_init(struct tcf_proto *tp) > root_ht->refcnt++; > root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000; > root_ht->prio = tp->prio; > + root_ht->is_root = true; > idr_init(&root_ht->handle_idr); > > if (tp_c == NULL) { > @@ -693,7 +695,7 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last, > goto out; > } > > - if (root_ht == ht) { > + if (ht->is_root) { > NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node"); > return -EINVAL; > } > @@ -795,6 +797,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, > NL_SET_ERR_MSG_MOD(extack, "Link hash table not found"); > return -EINVAL; > } > + if (ht_down->is_root) { > + NL_SET_ERR_MSG_MOD(extack, "Not linking to root node"); > + return -EINVAL; > + } > ht_down->refcnt++; Looks good to me ;-> cheers, jamal
On Thu, Sep 6, 2018 at 3:59 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > index 3f985f29ef30..d14048e38b5c 100644 > --- a/net/sched/cls_u32.c > +++ b/net/sched/cls_u32.c > @@ -84,6 +84,7 @@ struct tc_u_hnode { > int refcnt; > unsigned int divisor; > struct idr handle_idr; > + bool is_root; > struct rcu_head rcu; > u32 flags; > /* The 'ht' field MUST be the last field in structure to allow for > @@ -377,6 +378,7 @@ static int u32_init(struct tcf_proto *tp) > root_ht->refcnt++; > root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000; > root_ht->prio = tp->prio; > + root_ht->is_root = true; > idr_init(&root_ht->handle_idr); > > if (tp_c == NULL) { > @@ -693,7 +695,7 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last, > goto out; > } > > - if (root_ht == ht) { > + if (ht->is_root) { What's wrong with comparing pointers with root ht? > NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node"); > return -EINVAL; > } > @@ -795,6 +797,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, > NL_SET_ERR_MSG_MOD(extack, "Link hash table not found"); > return -EINVAL; > } > + if (ht_down->is_root) { root ht is saved in tp->root, so you can compare ht_down with it too, if you want. If this check is all what you need, you don't need an extra flag.
On Thu, Sep 06, 2018 at 07:57:25PM -0700, Cong Wang wrote: > > - if (root_ht == ht) { > > + if (ht->is_root) { > > > What's wrong with comparing pointers with root ht? The fact that there may be more than one tcf_proto sharing tp->data. > > NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node"); > > return -EINVAL; > > } > > @@ -795,6 +797,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, > > NL_SET_ERR_MSG_MOD(extack, "Link hash table not found"); > > return -EINVAL; > > } > > + if (ht_down->is_root) { > > root ht is saved in tp->root, so you can compare ht_down with it too, > if you want. > > If this check is all what you need, you don't need an extra flag. Again, *which* tp? We can trivially check that we are not linking to/deleting our own root, sure. But there's nothing to stop doing the same via another tcf_proto...
On Thu, Sep 6, 2018 at 8:04 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Thu, Sep 06, 2018 at 07:57:25PM -0700, Cong Wang wrote: > > > > - if (root_ht == ht) { > > > + if (ht->is_root) { > > > > > > What's wrong with comparing pointers with root ht? > > The fact that there may be more than one tcf_proto sharing tp->data. Hmm? root ht is from tp->root, not tp->data. Also, this very important information is missing in your one-line changelog... > > > > NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node"); > > > return -EINVAL; > > > } > > > @@ -795,6 +797,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, > > > NL_SET_ERR_MSG_MOD(extack, "Link hash table not found"); > > > return -EINVAL; > > > } > > > + if (ht_down->is_root) { > > > > root ht is saved in tp->root, so you can compare ht_down with it too, > > if you want. > > > > If this check is all what you need, you don't need an extra flag. > > Again, *which* tp? We can trivially check that we are not linking to/deleting Pretty sure there is a 'tp' in u32_set_parms() parameter list. Are you saying it is not what you want? If so, why? More importantly, why this information is again missing in your changelog? This patch is definitely not trivial, it deserves a detailed changelog. > our own root, sure. But there's nothing to stop doing the same via another > tcf_proto... To my best knowledge, the place where you set ->is_root=true is precisely same with where we set tp->root=root_ht, and it doesn't change after set. What am I missing here?
On Thu, Sep 06, 2018 at 08:23:36PM -0700, Cong Wang wrote: > Pretty sure there is a 'tp' in u32_set_parms() parameter list. > > Are you saying it is not what you want? If so, why? > > More importantly, why this information is again missing in your > changelog? This patch is definitely not trivial, it deserves a detailed > changelog. > > > > our own root, sure. But there's nothing to stop doing the same via another > > tcf_proto... > > To my best knowledge, the place where you set ->is_root=true > is precisely same with where we set tp->root=root_ht, and it doesn't > change after set. What am I missing here? The fact that there can be two (or more) different tcf_proto instances sharing ->data, but not ->root. And since ->data is shared, u32_get() on one tp will be able to return you ->root of *ANOTHER* one. So comparison with tp->root doesn't protect you. Try this on mainline: tc qdisc add dev eth0 ingress tc filter add dev eth0 parent ffff: protocol ip prio 100 handle 1: u32 divisor 1 tc filter add dev eth0 parent ffff: protocol ip prio 200 handle 2: u32 divisor 1 tc filter delete dev eth0 parent ffff: protocol ip prio 100 handle 801: u32 and watch the fun as soon as you get an incoming packet on eth0. That panic is fixed by 1/7, but you get "Not allowed to delete root node" for removing _your_ root, with "Can not delete in-use filter" for other's root (as in the last line of the reproducer).
On Thu, Sep 6, 2018 at 8:50 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Thu, Sep 06, 2018 at 08:23:36PM -0700, Cong Wang wrote: > > > Pretty sure there is a 'tp' in u32_set_parms() parameter list. > > > > Are you saying it is not what you want? If so, why? > > > > More importantly, why this information is again missing in your > > changelog? This patch is definitely not trivial, it deserves a detailed > > changelog. > > > > > > > our own root, sure. But there's nothing to stop doing the same via another > > > tcf_proto... > > > > To my best knowledge, the place where you set ->is_root=true > > is precisely same with where we set tp->root=root_ht, and it doesn't > > change after set. What am I missing here? > > The fact that there can be two (or more) different tcf_proto instances sharing > ->data, but not ->root. And since ->data is shared, u32_get() on one tp They have to share tp->data, that is how we link hashtables together. > will be able to return you ->root of *ANOTHER* one. So comparison with > tp->root doesn't protect you. Try this on mainline: Hmm, it is not u32_get(), it is u32_lookup_ht() which could get another root ht... I see. > > tc qdisc add dev eth0 ingress > tc filter add dev eth0 parent ffff: protocol ip prio 100 handle 1: u32 divisor 1 > tc filter add dev eth0 parent ffff: protocol ip prio 200 handle 2: u32 divisor 1 > tc filter delete dev eth0 parent ffff: protocol ip prio 100 handle 801: u32 > > and watch the fun as soon as you get an incoming packet on eth0. That panic > is fixed by 1/7, but you get "Not allowed to delete root node" for removing > _your_ root, with "Can not delete in-use filter" for other's root (as in the > last line of the reproducer). Sure, please consider: 1. adding such a test case to tools/testing/selftests/tc-testing/ 2. adding it in your changelog This would save a lot of time for both of us. I don't need to ask you for this if it is in the changelog, you don't have to explain it again. This is a win-win.
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index 3f985f29ef30..9ea5f2be907b 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -91,6 +91,7 @@ struct tc_u_hnode { */ struct tc_u_knode __rcu *ht[1]; }; +#define TCA_CLS_FLAGS_U32_ROOT (1<<8) struct tc_u_common { struct tc_u_hnode __rcu *hlist; @@ -377,6 +378,7 @@ static int u32_init(struct tcf_proto *tp) root_ht->refcnt++; root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000; root_ht->prio = tp->prio; + root_ht->flags = TCA_CLS_FLAGS_U32_ROOT; idr_init(&root_ht->handle_idr); if (tp_c == NULL) { @@ -491,7 +493,8 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h, struct tcf_block *block = tp->chain->block; struct tc_cls_u32_offload cls_u32 = {}; - tc_cls_common_offload_init(&cls_u32.common, tp, h->flags, extack); + tc_cls_common_offload_init(&cls_u32.common, tp, + h->flags & ~TCA_CLS_FLAGS_U32_ROOT, extack); cls_u32.command = TC_CLSU32_DELETE_HNODE; cls_u32.hnode.divisor = h->divisor; cls_u32.hnode.handle = h->handle; @@ -693,7 +696,7 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last, goto out; } - if (root_ht == ht) { + if (ht->flags & TCA_CLS_FLAGS_U32_ROOT) { NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node"); return -EINVAL; } @@ -795,6 +798,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp, NL_SET_ERR_MSG_MOD(extack, "Link hash table not found"); return -EINVAL; } + if (ht_down->flags & TCA_CLS_FLAGS_U32_ROOT) { + NL_SET_ERR_MSG_MOD(extack, "Not linke to root node"); + return -EINVAL; + } ht_down->refcnt++; } @@ -1214,7 +1221,8 @@ static int u32_reoffload_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht, struct tc_cls_u32_offload cls_u32 = {}; int err; - tc_cls_common_offload_init(&cls_u32.common, tp, ht->flags, extack); + tc_cls_common_offload_init(&cls_u32.common, tp, + ht->flags & ~TCA_CLS_FLAGS_U32_ROOT, extack); cls_u32.command = add ? TC_CLSU32_NEW_HNODE : TC_CLSU32_DELETE_HNODE; cls_u32.hnode.divisor = ht->divisor; cls_u32.hnode.handle = ht->handle;