| Message ID | 20180109140731.1022-9-jiri@resnulli.us |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: sched: allow qdiscs to share filter block instances | expand |
On 1/9/18 7:07 AM, Jiri Pirko wrote: > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h > index 9c026d9..038cde7 100644 > --- a/include/uapi/linux/rtnetlink.h > +++ b/include/uapi/linux/rtnetlink.h > @@ -150,6 +150,12 @@ enum { > RTM_NEWCACHEREPORT = 96, > #define RTM_NEWCACHEREPORT RTM_NEWCACHEREPORT > > + RTM_NEWBLOCK = 100, > +#define RTM_NEWBLOCK RTM_NEWBLOCK > + RTM_DELBLOCK, > +#define RTM_DELBLOCK RTM_DELBLOCK > + RTM_GETBLOCK, > +#define RTM_GETBLOCK RTM_GETBLOCK > __RTM_MAX, > #define RTM_MAX (((__RTM_MAX + 3) & ~3) - 1) > }; Seems like this is creating an inconsistency. RTM_GETBLOCK is used to dump the set of shared blocks, but RTM_NEWBLOCK / RTM_DELBLOCK are not used to create / delete one.
Wed, Jan 10, 2018 at 05:48:09PM CET, dsahern@gmail.com wrote: >On 1/9/18 7:07 AM, Jiri Pirko wrote: >> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h >> index 9c026d9..038cde7 100644 >> --- a/include/uapi/linux/rtnetlink.h >> +++ b/include/uapi/linux/rtnetlink.h >> @@ -150,6 +150,12 @@ enum { >> RTM_NEWCACHEREPORT = 96, >> #define RTM_NEWCACHEREPORT RTM_NEWCACHEREPORT >> >> + RTM_NEWBLOCK = 100, >> +#define RTM_NEWBLOCK RTM_NEWBLOCK >> + RTM_DELBLOCK, >> +#define RTM_DELBLOCK RTM_DELBLOCK >> + RTM_GETBLOCK, >> +#define RTM_GETBLOCK RTM_GETBLOCK >> __RTM_MAX, >> #define RTM_MAX (((__RTM_MAX + 3) & ~3) - 1) >> }; > >Seems like this is creating an inconsistency. RTM_GETBLOCK is used to >dump the set of shared blocks, but RTM_NEWBLOCK / RTM_DELBLOCK are not >used to create / delete one. Why is it a problem? RTM_NEWBLOCK is used as a reply for RTM_GETBLOCK. I plan to have block notifications as a follow-up, there the RTM_GETBLOCK and RTM_DELBLOCK will be used. The fact the user cannot create and delete block explicitly is no problem in my opinion. The block creation and deletion is done according to usage of qdiscs.
Thu, Jan 11, 2018 at 10:37:10AM CET, jiri@resnulli.us wrote: >Wed, Jan 10, 2018 at 05:48:09PM CET, dsahern@gmail.com wrote: >>On 1/9/18 7:07 AM, Jiri Pirko wrote: >>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h >>> index 9c026d9..038cde7 100644 >>> --- a/include/uapi/linux/rtnetlink.h >>> +++ b/include/uapi/linux/rtnetlink.h >>> @@ -150,6 +150,12 @@ enum { >>> RTM_NEWCACHEREPORT = 96, >>> #define RTM_NEWCACHEREPORT RTM_NEWCACHEREPORT >>> >>> + RTM_NEWBLOCK = 100, >>> +#define RTM_NEWBLOCK RTM_NEWBLOCK >>> + RTM_DELBLOCK, >>> +#define RTM_DELBLOCK RTM_DELBLOCK >>> + RTM_GETBLOCK, >>> +#define RTM_GETBLOCK RTM_GETBLOCK >>> __RTM_MAX, >>> #define RTM_MAX (((__RTM_MAX + 3) & ~3) - 1) >>> }; >> >>Seems like this is creating an inconsistency. RTM_GETBLOCK is used to >>dump the set of shared blocks, but RTM_NEWBLOCK / RTM_DELBLOCK are not >>used to create / delete one. > >Why is it a problem? RTM_NEWBLOCK is used as a reply for RTM_GETBLOCK. >I plan to have block notifications as a follow-up, there the RTM_GETBLOCK I mean RTM_NEWBLOCK and RTM_DELBLOCK of couse. >and RTM_DELBLOCK will be used. The fact the user cannot create and >delete block explicitly is no problem in my opinion. The block creation >and deletion is done according to usage of qdiscs.
On 18-01-09 09:07 AM, Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Add simple block get operation which primary purpose is to check the > block existence by block index. > block_dump missing? cheers, jamal
Thu, Jan 11, 2018 at 02:27:11PM CET, jhs@mojatatu.com wrote: >On 18-01-09 09:07 AM, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Add simple block get operation which primary purpose is to check the >> block existence by block index. >> > >block_dump missing? It is not needed for anything now. You see all the blocks when you list qdiscs. Yet, dump could be easily added if needed in the future.
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index 9c026d9..038cde7 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -150,6 +150,12 @@ enum { RTM_NEWCACHEREPORT = 96, #define RTM_NEWCACHEREPORT RTM_NEWCACHEREPORT + RTM_NEWBLOCK = 100, +#define RTM_NEWBLOCK RTM_NEWBLOCK + RTM_DELBLOCK, +#define RTM_DELBLOCK RTM_DELBLOCK + RTM_GETBLOCK, +#define RTM_GETBLOCK RTM_GETBLOCK __RTM_MAX, #define RTM_MAX (((__RTM_MAX + 3) & ~3) - 1) }; diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index d687e58..14e4f20 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -1553,6 +1553,69 @@ int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts, } EXPORT_SYMBOL(tc_setup_cb_call); +static int block_notify_fill(struct net *net, struct sk_buff *skb, + struct tcf_block *block, u32 portid, u32 seq, + u16 flags, int event) +{ + struct nlmsghdr *nlh; + struct tcmsg *tcm; + + nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags); + if (!nlh) + return -EMSGSIZE; + tcm = nlmsg_data(nlh); + memset(tcm, 0, sizeof(*tcm)); + tcm->tcm_family = AF_UNSPEC; + tcm->tcm_ifindex = TCM_IFINDEX_MAGIC_BLOCK; + tcm->tcm_block_index = block->index; + return 0; +} + +static int block_notify(struct net *net, struct sk_buff *oskb, + struct nlmsghdr *n, struct tcf_block *block, int event) +{ + u32 portid = NETLINK_CB(oskb).portid; + struct sk_buff *skb; + int err; + + skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL); + if (!skb) + return -ENOBUFS; + + err = block_notify_fill(net, skb, block, portid, + n->nlmsg_seq, n->nlmsg_flags, event); + if (err) { + kfree_skb(skb); + return err; + } + + return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT); +} + +static int tc_ctl_block(struct sk_buff *skb, struct nlmsghdr *n, + struct netlink_ext_ack *extack) +{ + struct net *net = sock_net(skb->sk); + struct tcf_block *block; + struct tcmsg *tcm; + + if (n->nlmsg_len < nlmsg_msg_size(sizeof(*tcm))) + return -EINVAL; + + tcm = nlmsg_data(n); + + if (tcm->tcm_ifindex != TCM_IFINDEX_MAGIC_BLOCK) + return -EINVAL; + + block = tcf_block_lookup(net, tcm->tcm_block_index); + if (!block) { + NL_SET_ERR_MSG(extack, "Block with the given index does not exist"); + return -EINVAL; + } + + return block_notify(net, skb, n, block, RTM_NEWBLOCK); +} + static __net_init int tcf_net_init(struct net *net) { struct tcf_net *tn = net_generic(net, tcf_net_id); @@ -1591,6 +1654,7 @@ static int __init tc_filter_init(void) rtnl_register(PF_UNSPEC, RTM_DELTFILTER, tc_ctl_tfilter, NULL, 0); rtnl_register(PF_UNSPEC, RTM_GETTFILTER, tc_ctl_tfilter, tc_dump_tfilter, 0); + rtnl_register(PF_UNSPEC, RTM_GETBLOCK, tc_ctl_block, NULL, 0); return 0; } diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c index 7b7433a..4e95a46 100644 --- a/security/selinux/nlmsgtab.c +++ b/security/selinux/nlmsgtab.c @@ -80,6 +80,9 @@ static const struct nlmsg_perm nlmsg_route_perms[] = { RTM_NEWSTATS, NETLINK_ROUTE_SOCKET__NLMSG_READ }, { RTM_GETSTATS, NETLINK_ROUTE_SOCKET__NLMSG_READ }, { RTM_NEWCACHEREPORT, NETLINK_ROUTE_SOCKET__NLMSG_READ }, + { RTM_NEWBLOCK, NETLINK_ROUTE_SOCKET__NLMSG_READ }, + { RTM_DELBLOCK, NETLINK_ROUTE_SOCKET__NLMSG_READ }, + { RTM_GETBLOCK, NETLINK_ROUTE_SOCKET__NLMSG_READ }, }; static const struct nlmsg_perm nlmsg_tcpdiag_perms[] = @@ -159,7 +162,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm) switch (sclass) { case SECCLASS_NETLINK_ROUTE_SOCKET: /* RTM_MAX always point to RTM_SETxxxx, ie RTM_NEWxxx + 3 */ - BUILD_BUG_ON(RTM_MAX != (RTM_NEWCACHEREPORT + 3)); + BUILD_BUG_ON(RTM_MAX != (RTM_NEWBLOCK + 3)); err = nlmsg_perm(nlmsg_type, perm, nlmsg_route_perms, sizeof(nlmsg_route_perms)); break;