| Message ID | 20230331235528.1106675-2-anjali.k.kulkarni@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Process connector bug fixes & enhancements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Fri, 31 Mar 2023 16:55:23 -0700 Anjali Kulkarni wrote: > To use filtering at the connector & cn_proc layers, we need to enable > filtering in the netlink layer. This reverses the patch which removed > netlink filtering. > > Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com> Acked-by: Jakub Kicinski <kuba@kernel.org>
On Fri, 31 Mar 2023 16:55:23 -0700 Anjali Kulkarni wrote: > +int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, > + __u32 portid, __u32 group, gfp_t allocation, > + int (*filter)(struct sock *dsk, > + struct sk_buff *skb, void *data), > + void *filter_data); > -int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid, > - u32 group, gfp_t allocation) > +int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, > + u32 portid, > + u32 group, gfp_t allocation, > + int (*filter)(struct sock *dsk, > + struct sk_buff *skb, void *data), > + void *filter_data) nit: slight divergence between __u32 and u32 types, something to clean up if you post v5
> On Mar 31, 2023, at 9:09 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 31 Mar 2023 16:55:23 -0700 Anjali Kulkarni wrote: >> +int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, >> + __u32 portid, __u32 group, gfp_t allocation, >> + int (*filter)(struct sock *dsk, >> + struct sk_buff *skb, void *data), >> + void *filter_data); > >> -int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid, >> - u32 group, gfp_t allocation) >> +int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, >> + u32 portid, >> + u32 group, gfp_t allocation, >> + int (*filter)(struct sock *dsk, >> + struct sk_buff *skb, void *data), >> + void *filter_data) > > nit: slight divergence between __u32 and u32 types, something to clean > up if you post v5 Thanks so much! Will do. Any comments on the connector patches? Anjali
On Sat, 1 Apr 2023 18:24:11 +0000 Anjali Kulkarni wrote: > > nit: slight divergence between __u32 and u32 types, something to clean > > up if you post v5 > > Thanks so much! Will do. Any comments on the connector patches? patch 3 looks fine as far as I can read thru the ugly in place casts patch 5 looks a bit connector specific, no idea :S patch 6 does seem to lift the NET_ADMIN for group 0 and from &init_user_ns, CAP_NET_ADMIN to net->user_ns, CAP_NET_ADMIN whether that's right or not I have no idea :( Also, BTW, on the coding level: +static int cn_bind(struct net *net, int group) +{ + unsigned long groups = 0; + groups = (unsigned long) group; + + if (test_bit(CN_IDX_PROC - 1, &groups)) Why not just +static int cn_bind(struct net *net, int group) +{ + if (group == CN_IDX_PROC) ? Who are you hoping will merge this?
> On Apr 1, 2023, at 12:12 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Sat, 1 Apr 2023 18:24:11 +0000 Anjali Kulkarni wrote: >>> nit: slight divergence between __u32 and u32 types, something to clean >>> up if you post v5 >> >> Thanks so much! Will do. Any comments on the connector patches? > > patch 3 looks fine as far as I can read thru the ugly in place casts Thanks for reviewing! > patch 5 looks a bit connector specific, no idea :S > patch 6 does seem to lift the NET_ADMIN for group 0 > and from &init_user_ns, CAP_NET_ADMIN to net->user_ns, CAP_NET_ADMIN > whether that's right or not I have no idea :( I can move this back to &init_user_ns, and will look at group 0 too. > > Also, BTW, on the coding level: > > +static int cn_bind(struct net *net, int group) > +{ > + unsigned long groups = 0; > + groups = (unsigned long) group; > + > + if (test_bit(CN_IDX_PROC - 1, &groups)) > > Why not just > > +static int cn_bind(struct net *net, int group) > +{ > + if (group == CN_IDX_PROC) > > ? Will change this. > > Who are you hoping will merge this? I am not sure. Whom should I contact to move this forward?
> > Who are you hoping will merge this? Could I request you to look into merging the patches which seem ok to you, since you are listed as the maintainer for these? I can make any more changes for the connector patches if you see the need.. Anjali
On Sat, 1 Apr 2023 19:58:31 +0000 Anjali Kulkarni wrote: > > patch 5 looks a bit connector specific, no idea :S > > patch 6 does seem to lift the NET_ADMIN for group 0 > > and from &init_user_ns, CAP_NET_ADMIN to net->user_ns, CAP_NET_ADMIN > > whether that's right or not I have no idea :( > I can move this back to &init_user_ns, and will look at group 0 too. Just to be clear - I wasn't saying that it's incorrect, I simply don't know :)
On Sun, 2 Apr 2023 02:32:19 +0000 Anjali Kulkarni wrote: > > Who are you hoping will merge this? > Could I request you to look into merging the patches which seem ok to > you, since you are listed as the maintainer for these? I can make any > more changes for the connector patches if you see the need.. The first two, you mean? We can merge them, but we need to know that the rest is also going somewhere. Kernel has a rule against merging APIs without any in-tree users, so we need a commitment that the user will also reach linux-next before the merge window :( Christian was commenting on previous releases maybe he can take or just review the last 4 patches?
> On Apr 3, 2023, at 1:50 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Sun, 2 Apr 2023 02:32:19 +0000 Anjali Kulkarni wrote: >>> Who are you hoping will merge this? >> Could I request you to look into merging the patches which seem ok to >> you, since you are listed as the maintainer for these? I can make any >> more changes for the connector patches if you see the need.. > > The first two, you mean? We can merge them, but we need to know that Yes, even perhaps the first 3:-), since the third one has bug fixes which looked ok to you? > the rest is also going somewhere. Kernel has a rule against merging > APIs without any in-tree users, so we need a commitment that the > user will also reach linux-next before the merge window :( Yes, sounds right. > > Christian was commenting on previous releases maybe he can take or just > review the last 4 patches? Sounds fine too. I hope Christian can review these. Anjali
> On Apr 3, 2023, at 1:50 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Sun, 2 Apr 2023 02:32:19 +0000 Anjali Kulkarni wrote: >>> Who are you hoping will merge this? >> Could I request you to look into merging the patches which seem ok to >> you, since you are listed as the maintainer for these? I can make any >> more changes for the connector patches if you see the need.. > > The first two, you mean? We can merge them, but we need to know that > the rest is also going somewhere. Kernel has a rule against merging > APIs without any in-tree users, so we need a commitment that the > user will also reach linux-next before the merge window :( > Jakub, could you please look into reviewing patches 3,4 & 5 as well? Patch 4 is just test code. Patch 3 is fixing bug fixes in current code so should be good to have - also it is not too connector specific. I can explain patch 5 in more detail if needed. > Christian was commenting on previous releases maybe he can take or just > review the last 4 patches? Christian, could you please look into reviewing patch 6? This just deals with who can get the exit notifications. Thanks Anjali
On Wed, 26 Apr 2023 23:58:55 +0000 Anjali Kulkarni wrote: > Jakub, could you please look into reviewing patches 3,4 & 5 as well? > Patch 4 is just test code. Patch 3 is fixing bug fixes in current > code so should be good to have - also it is not too connector > specific. I can explain patch 5 in more detail if needed. I don't have sufficient knowledge to review that code, sorry :(
> On Apr 27, 2023, at 10:03 AM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 26 Apr 2023 23:58:55 +0000 Anjali Kulkarni wrote: >> Jakub, could you please look into reviewing patches 3,4 & 5 as well? >> Patch 4 is just test code. Patch 3 is fixing bug fixes in current >> code so should be good to have - also it is not too connector >> specific. I can explain patch 5 in more detail if needed. > > I don't have sufficient knowledge to review that code, sorry :( Is there anyone who can please help review this code? Christian, could you please help take a look? Thanks Anjali
> On May 11, 2023, at 9:04 AM, Anjali Kulkarni <Anjali.K.Kulkarni@oracle.com> wrote: > > > >> On Apr 27, 2023, at 10:03 AM, Jakub Kicinski <kuba@kernel.org> wrote: >> >> On Wed, 26 Apr 2023 23:58:55 +0000 Anjali Kulkarni wrote: >>> Jakub, could you please look into reviewing patches 3,4 & 5 as well? >>> Patch 4 is just test code. Patch 3 is fixing bug fixes in current >>> code so should be good to have - also it is not too connector >>> specific. I can explain patch 5 in more detail if needed. >> >> I don't have sufficient knowledge to review that code, sorry :( > > Is there anyone who can please help review this code? > Christian, could you please help take a look? > Thanks > Anjali > Gentle ping again - Christian could you please help review? Anjali
On Thu, 1 Jun 2023 16:15:31 +0000 Anjali Kulkarni wrote: > >> I don't have sufficient knowledge to review that code, sorry :( > > > > Is there anyone who can please help review this code? > > Christian, could you please help take a look? > > Gentle ping again - Christian could you please help review? The code may have security implications, I really don't feel like I can be the sole reviewer. There's a bunch of experts working at Oracle, maybe you could get one of them to put their name on it? I can apply the patches, I just want to be sure I'm not the _only_ reviewer.
> On Jun 1, 2023, at 9:24 AM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 1 Jun 2023 16:15:31 +0000 Anjali Kulkarni wrote: >>>> I don't have sufficient knowledge to review that code, sorry :( >>> >>> Is there anyone who can please help review this code? >>> Christian, could you please help take a look? >> >> Gentle ping again - Christian could you please help review? > > The code may have security implications, I really don't feel like I can > be the sole reviewer. There's a bunch of experts working at Oracle, > maybe you could get one of them to put their name on it? I can apply > the patches, I just want to be sure I'm not the _only_ reviewer. Thanks so much for your response. There is someone at Oracle who looked at this some time ago and is familiar enough with this to review the code - but he is not a kernel committer - he sends occasional patches upstream which get committed - would it be ok if he reviewed it along with you and then you could commit it? If you know of someone from Oracle who could also potentially review it, please let me know.
On Thu, 1 Jun 2023 16:34:08 +0000 Anjali Kulkarni wrote: > > The code may have security implications, I really don't feel like I can > > be the sole reviewer. There's a bunch of experts working at Oracle, > > maybe you could get one of them to put their name on it? I can apply > > the patches, I just want to be sure I'm not the _only_ reviewer. > > Thanks so much for your response. There is someone at Oracle who > looked at this some time ago and is familiar enough with this to > review the code - but he is not a kernel committer - he sends > occasional patches upstream which get committed - would it be ok if > he reviewed it along with you and then you could commit it? If you > know of someone from Oracle who could also potentially review it, > please let me know. I meant someone seasoned. IMHO one of the benefits of employing upstream experts for corporation like Oracle should be that you can lean on them for reviews: $ git log --format='%ae' --author='Oracle' --since='2 years ago' | sort | uniq -c | sort -rn 811 willy@infradead.org 312 rmk+kernel@armlinux.org.uk 91 Liam.Howlett@Oracle.com 60 vishal.moola@gmail.com $ git log --format='%ae' --author='@oracle.com' --since='2 years ago' | sort | uniq -c | sort -rn | head -10 451 chuck.lever@oracle.com 154 michael.christie@oracle.com 118 nick.alcock@oracle.com 71 martin.petersen@oracle.com 59 mike.kravetz@oracle.com 58 sidhartha.kumar@oracle.com 55 liam.howlett@oracle.com 53 anand.jain@oracle.com 32 dai.ngo@oracle.com 32 allison.henderson@oracle.com
> On Jun 1, 2023, at 9:43 AM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 1 Jun 2023 16:34:08 +0000 Anjali Kulkarni wrote: >>> The code may have security implications, I really don't feel like I can >>> be the sole reviewer. There's a bunch of experts working at Oracle, >>> maybe you could get one of them to put their name on it? I can apply >>> the patches, I just want to be sure I'm not the _only_ reviewer. >> >> Thanks so much for your response. There is someone at Oracle who >> looked at this some time ago and is familiar enough with this to >> review the code - but he is not a kernel committer - he sends >> occasional patches upstream which get committed - would it be ok if >> he reviewed it along with you and then you could commit it? If you >> know of someone from Oracle who could also potentially review it, >> please let me know. > > I meant someone seasoned. IMHO one of the benefits of employing > upstream experts for corporation like Oracle should be that you > can lean on them for reviews: > > $ git log --format='%ae' --author='Oracle' --since='2 years ago' | sort | uniq -c | sort -rn > 811 willy@infradead.org > 312 rmk+kernel@armlinux.org.uk > 91 Liam.Howlett@Oracle.com > 60 vishal.moola@gmail.com > > $ git log --format='%ae' --author='@oracle.com' --since='2 years ago' | sort | uniq -c | sort -rn | head -10 > 451 chuck.lever@oracle.com > 154 michael.christie@oracle.com > 118 nick.alcock@oracle.com > 71 martin.petersen@oracle.com > 59 mike.kravetz@oracle.com > 58 sidhartha.kumar@oracle.com > 55 liam.howlett@oracle.com > 53 anand.jain@oracle.com > 32 dai.ngo@oracle.com > 32 allison.henderson@oracle.com Thanks, let me check. Anjali
diff --git a/include/linux/netlink.h b/include/linux/netlink.h index c43ac7690eca..866bbc5a4c8d 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -206,6 +206,11 @@ bool netlink_strict_get_check(struct sk_buff *skb); int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 portid, int nonblock); int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 portid, __u32 group, gfp_t allocation); +int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, + __u32 portid, __u32 group, gfp_t allocation, + int (*filter)(struct sock *dsk, + struct sk_buff *skb, void *data), + void *filter_data); int netlink_set_err(struct sock *ssk, __u32 portid, __u32 group, int code); int netlink_register_notifier(struct notifier_block *nb); int netlink_unregister_notifier(struct notifier_block *nb); diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index c64277659753..003c7e6ec9be 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1432,6 +1432,8 @@ struct netlink_broadcast_data { int delivered; gfp_t allocation; struct sk_buff *skb, *skb2; + int (*tx_filter)(struct sock *dsk, struct sk_buff *skb, void *data); + void *tx_data; }; static void do_one_broadcast(struct sock *sk, @@ -1485,6 +1487,11 @@ static void do_one_broadcast(struct sock *sk, p->delivery_failure = 1; goto out; } + if (p->tx_filter && p->tx_filter(sk, p->skb2, p->tx_data)) { + kfree_skb(p->skb2); + p->skb2 = NULL; + goto out; + } if (sk_filter(sk, p->skb2)) { kfree_skb(p->skb2); p->skb2 = NULL; @@ -1507,8 +1514,12 @@ static void do_one_broadcast(struct sock *sk, sock_put(sk); } -int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid, - u32 group, gfp_t allocation) +int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, + u32 portid, + u32 group, gfp_t allocation, + int (*filter)(struct sock *dsk, + struct sk_buff *skb, void *data), + void *filter_data) { struct net *net = sock_net(ssk); struct netlink_broadcast_data info; @@ -1527,6 +1538,8 @@ int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid, info.allocation = allocation; info.skb = skb; info.skb2 = NULL; + info.tx_filter = filter; + info.tx_data = filter_data; /* While we sleep in clone, do not allow to change socket list */ @@ -1552,6 +1565,14 @@ int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid, } return -ESRCH; } +EXPORT_SYMBOL(netlink_broadcast_filtered); + +int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 portid, + u32 group, gfp_t allocation) +{ + return netlink_broadcast_filtered(ssk, skb, portid, group, allocation, + NULL, NULL); +} EXPORT_SYMBOL(netlink_broadcast); struct netlink_set_err_data {
To use filtering at the connector & cn_proc layers, we need to enable filtering in the netlink layer. This reverses the patch which removed netlink filtering. Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com> --- include/linux/netlink.h | 5 +++++ net/netlink/af_netlink.c | 25 +++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-)