| Message ID | 1479334570-25159-1-git-send-email-mrv@mojatatu.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Roman Mashak <mrv@mojatatu.com> Date: Wed, 16 Nov 2016 17:16:10 -0500 > Userland client should be able to read an event, and reflect it back to > the kernel, therefore it needs to extract complete set of netlink flags. > > For example, this will allow "tc monitor" to distinguish Add and Replace > operations. > > Signed-off-by: Roman Mashak <mrv@mojatatu.com> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> Looks good, applied.
On Wed, Nov 16, 2016 at 2:16 PM, Roman Mashak <mrv@mojatatu.com> wrote: > Userland client should be able to read an event, and reflect it back to > the kernel, therefore it needs to extract complete set of netlink flags. > > For example, this will allow "tc monitor" to distinguish Add and Replace > operations. > > Signed-off-by: Roman Mashak <mrv@mojatatu.com> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> > --- > net/sched/cls_api.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 2b2a797..8e93d4a 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -112,7 +112,7 @@ static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb, > > for (it_chain = chain; (tp = rtnl_dereference(*it_chain)) != NULL; > it_chain = &tp->next) > - tfilter_notify(net, oskb, n, tp, 0, event, false); > + tfilter_notify(net, oskb, n, tp, n->nlmsg_flags, event, false); I must miss something, why does it make sense to pass n->nlmsg_flags as 'fh' to tfilter_notify()?? > } > > /* Select new prio value from the range, managed by kernel. */ > @@ -430,7 +430,8 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb, > if (!skb) > return -ENOBUFS; > > - if (tcf_fill_node(net, skb, tp, fh, portid, n->nlmsg_seq, 0, event) <= 0) { > + if (tcf_fill_node(net, skb, tp, fh, portid, n->nlmsg_seq, > + n->nlmsg_flags, event) <= 0) { This part makes sense.
On Thu, Nov 17, 2016 at 1:02 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Wed, Nov 16, 2016 at 2:16 PM, Roman Mashak <mrv@mojatatu.com> wrote: >> Userland client should be able to read an event, and reflect it back to >> the kernel, therefore it needs to extract complete set of netlink flags. >> >> For example, this will allow "tc monitor" to distinguish Add and Replace >> operations. >> >> Signed-off-by: Roman Mashak <mrv@mojatatu.com> >> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> >> --- >> net/sched/cls_api.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >> index 2b2a797..8e93d4a 100644 >> --- a/net/sched/cls_api.c >> +++ b/net/sched/cls_api.c >> @@ -112,7 +112,7 @@ static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb, >> >> for (it_chain = chain; (tp = rtnl_dereference(*it_chain)) != NULL; >> it_chain = &tp->next) >> - tfilter_notify(net, oskb, n, tp, 0, event, false); >> + tfilter_notify(net, oskb, n, tp, n->nlmsg_flags, event, false); > > > I must miss something, why does it make sense to pass n->nlmsg_flags > as 'fh' to tfilter_notify()?? Ping... Any response? It still doesn't look correct to me. I will send a fix unless someone could explain this.
On 11/22/2016 06:23 AM, Cong Wang wrote: > On Thu, Nov 17, 2016 at 1:02 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> On Wed, Nov 16, 2016 at 2:16 PM, Roman Mashak <mrv@mojatatu.com> wrote: >>> Userland client should be able to read an event, and reflect it back to >>> the kernel, therefore it needs to extract complete set of netlink flags. >>> >>> For example, this will allow "tc monitor" to distinguish Add and Replace >>> operations. >>> >>> Signed-off-by: Roman Mashak <mrv@mojatatu.com> >>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> >>> --- >>> net/sched/cls_api.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >>> index 2b2a797..8e93d4a 100644 >>> --- a/net/sched/cls_api.c >>> +++ b/net/sched/cls_api.c >>> @@ -112,7 +112,7 @@ static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb, >>> >>> for (it_chain = chain; (tp = rtnl_dereference(*it_chain)) != NULL; >>> it_chain = &tp->next) >>> - tfilter_notify(net, oskb, n, tp, 0, event, false); >>> + tfilter_notify(net, oskb, n, tp, n->nlmsg_flags, event, false); >> >> >> I must miss something, why does it make sense to pass n->nlmsg_flags >> as 'fh' to tfilter_notify()?? > > Ping... Any response? > > It still doesn't look correct to me. I will send a fix unless someone could > explain this. Sigh, I missed that this was applied already to -net (it certainly doesn't look like -net material, but rather -net-next stuff) ... This definitely looks buggy to me, the 0 as it was before was correct here (as it means we delete the whole chain in this case). If you could send a patch would be great. Thanks Cong!
Daniel Borkmann <daniel@iogearbox.net> writes: > On 11/22/2016 06:23 AM, Cong Wang wrote: >> On Thu, Nov 17, 2016 at 1:02 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >>> On Wed, Nov 16, 2016 at 2:16 PM, Roman Mashak <mrv@mojatatu.com> wrote: >>>> Userland client should be able to read an event, and reflect it back to >>>> the kernel, therefore it needs to extract complete set of netlink flags. >>>> >>>> For example, this will allow "tc monitor" to distinguish Add and Replace >>>> operations. >>>> >>>> Signed-off-by: Roman Mashak <mrv@mojatatu.com> >>>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> >>>> --- >>>> net/sched/cls_api.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >>>> index 2b2a797..8e93d4a 100644 >>>> --- a/net/sched/cls_api.c >>>> +++ b/net/sched/cls_api.c >>>> @@ -112,7 +112,7 @@ static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb, >>>> >>>> for (it_chain = chain; (tp = rtnl_dereference(*it_chain)) != NULL; >>>> it_chain = &tp->next) >>>> - tfilter_notify(net, oskb, n, tp, 0, event, false); >>>> + tfilter_notify(net, oskb, n, tp, n->nlmsg_flags, event, false); >>> >>> >>> I must miss something, why does it make sense to pass n->nlmsg_flags >>> as 'fh' to tfilter_notify()?? >> >> Ping... Any response? >> >> It still doesn't look correct to me. I will send a fix unless someone could >> explain this. > > Sigh, I missed that this was applied already to -net (it certainly doesn't look > like -net material, but rather -net-next stuff) ... This definitely looks buggy > to me, the 0 as it was before was correct here (as it means we delete the whole > chain in this case). > > If you could send a patch would be great. Thanks Cong! Cong/Daniel, sorry for late response, I was distracted. I apologize, I will send a fix today.
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 2b2a797..8e93d4a 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -112,7 +112,7 @@ static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb, for (it_chain = chain; (tp = rtnl_dereference(*it_chain)) != NULL; it_chain = &tp->next) - tfilter_notify(net, oskb, n, tp, 0, event, false); + tfilter_notify(net, oskb, n, tp, n->nlmsg_flags, event, false); } /* Select new prio value from the range, managed by kernel. */ @@ -430,7 +430,8 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb, if (!skb) return -ENOBUFS; - if (tcf_fill_node(net, skb, tp, fh, portid, n->nlmsg_seq, 0, event) <= 0) { + if (tcf_fill_node(net, skb, tp, fh, portid, n->nlmsg_seq, + n->nlmsg_flags, event) <= 0) { kfree_skb(skb); return -EINVAL; }