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 <[email protected]>
---
include/linux/netlink.h | 5 +++++
net/netlink/af_netlink.c | 25 +++++++++++++++++++++++--
2 files changed, 28 insertions(+), 2 deletions(-)
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 {
--
2.40.0
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 <[email protected]>
Acked-by: Jakub Kicinski <[email protected]>
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 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 May 11, 2023, at 9:04 AM, Anjali Kulkarni <[email protected]> wrote:
>
>
>
>> On Apr 27, 2023, at 10:03 AM, Jakub Kicinski <[email protected]> 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: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 [email protected]
312 [email protected]
91 [email protected]
60 [email protected]
$ git log --format='%ae' --author='@oracle.com' --since='2 years ago' | sort | uniq -c | sort -rn | head -10
451 [email protected]
154 [email protected]
118 [email protected]
71 [email protected]
59 [email protected]
58 [email protected]
55 [email protected]
53 [email protected]
32 [email protected]
32 [email protected]
> On Jun 1, 2023, at 9:24 AM, Jakub Kicinski <[email protected]> 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 Jun 1, 2023, at 9:43 AM, Jakub Kicinski <[email protected]> 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 [email protected]
> 312 [email protected]
> 91 [email protected]
> 60 [email protected]
>
> $ git log --format='%ae' --author='@oracle.com' --since='2 years ago' | sort | uniq -c | sort -rn | head -10
> 451 [email protected]
> 154 [email protected]
> 118 [email protected]
> 71 [email protected]
> 59 [email protected]
> 58 [email protected]
> 55 [email protected]
> 53 [email protected]
> 32 [email protected]
> 32 [email protected]
Thanks, let me check.
Anjali