| Message ID | 20170530213325.21652-1-fbl@sysclose.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 30/05/2017 à 23:33, Flavio Leitner a écrit : > Don't include netns id for notifications broadcasts when the > socket and the skb are in the same netns because it will be > an error which can't be distinguished from a peer netns failing > to allocate an id. I don't understand the problem. peernet2id() doesn't allocate ids, it only do a lookup. If you need an id for the current netns, you have to allocate one. This patch changes the metadata exported to the userland and will break existing tools.
On Wed, May 31, 2017 at 10:38:21AM +0200, Nicolas Dichtel wrote: > Le 30/05/2017 à 23:33, Flavio Leitner a écrit : > > Don't include netns id for notifications broadcasts when the > > socket and the skb are in the same netns because it will be > > an error which can't be distinguished from a peer netns failing > > to allocate an id. > I don't understand the problem. peernet2id() doesn't allocate ids, it only do a > lookup. If you need an id for the current netns, you have to allocate one. The issue is that if you query an interface on the same netns, the error is returned, then we cannot tell if the iface is on the same netns or if there was an error while allocating the ID and the iface is on another netns. > This patch changes the metadata exported to the userland and will break existing > tools. It should not break because it changes only for interfaces on the same netns where there is no ID and that value wasn't exported until recently.
Le 31/05/2017 à 14:28, Flavio Leitner a écrit : > On Wed, May 31, 2017 at 10:38:21AM +0200, Nicolas Dichtel wrote: >> Le 30/05/2017 à 23:33, Flavio Leitner a écrit : >>> Don't include netns id for notifications broadcasts when the >>> socket and the skb are in the same netns because it will be >>> an error which can't be distinguished from a peer netns failing >>> to allocate an id. >> I don't understand the problem. peernet2id() doesn't allocate ids, it only do a >> lookup. If you need an id for the current netns, you have to allocate one. > > The issue is that if you query an interface on the same netns, the > error is returned, then we cannot tell if the iface is on the same > netns or if there was an error while allocating the ID and the > iface is on another netns. If the returned id is NETNSA_NSID_NOT_ASSIGNED, then the netns is the same. Some lines before your patch, we call peernet_has_id() when the netns differ, thus we ensure that the id is available. The principle was that netlink messages of other netns can be sent only if an id is assigned. > >> This patch changes the metadata exported to the userland and will break existing >> tools. > > It should not break because it changes only for interfaces on > the same netns where there is no ID and that value wasn't > exported until recently. > It was exported since the initial patch (59324cf35aba ("netlink: allow to listen "all" netns"). Am I wrong? Regards, Nicolas
On Wed, May 31, 2017 at 03:48:06PM +0200, Nicolas Dichtel wrote: > Le 31/05/2017 à 14:28, Flavio Leitner a écrit : > > On Wed, May 31, 2017 at 10:38:21AM +0200, Nicolas Dichtel wrote: > >> Le 30/05/2017 à 23:33, Flavio Leitner a écrit : > >>> Don't include netns id for notifications broadcasts when the > >>> socket and the skb are in the same netns because it will be > >>> an error which can't be distinguished from a peer netns failing > >>> to allocate an id. > >> I don't understand the problem. peernet2id() doesn't allocate ids, it only do a > >> lookup. If you need an id for the current netns, you have to allocate one. > > > > The issue is that if you query an interface on the same netns, the > > error is returned, then we cannot tell if the iface is on the same > > netns or if there was an error while allocating the ID and the > > iface is on another netns. > If the returned id is NETNSA_NSID_NOT_ASSIGNED, then the netns is the same. > > Some lines before your patch, we call peernet_has_id() when the netns differ, > thus we ensure that the id is available. Right, but that's internal to the kernel. > The principle was that netlink messages of other netns can be sent only if an id > is assigned. OK, could you please update include/uapi/linux/net_namespace.h to reflect that? It says NETNSA_NSID_NOT_ASSIGNED are attributes for RTM_NEWNSID or RTM_GETNSID which makes sense, but NOT_ASSIGNED sounds little like SAME_NSID for other message types.
Le 31/05/2017 à 20:34, Flavio Leitner a écrit : > On Wed, May 31, 2017 at 03:48:06PM +0200, Nicolas Dichtel wrote: >> Le 31/05/2017 à 14:28, Flavio Leitner a écrit : >>> On Wed, May 31, 2017 at 10:38:21AM +0200, Nicolas Dichtel wrote: >>>> Le 30/05/2017 à 23:33, Flavio Leitner a écrit : >>>>> Don't include netns id for notifications broadcasts when the >>>>> socket and the skb are in the same netns because it will be >>>>> an error which can't be distinguished from a peer netns failing >>>>> to allocate an id. >>>> I don't understand the problem. peernet2id() doesn't allocate ids, it only do a >>>> lookup. If you need an id for the current netns, you have to allocate one. >>> >>> The issue is that if you query an interface on the same netns, the >>> error is returned, then we cannot tell if the iface is on the same >>> netns or if there was an error while allocating the ID and the >>> iface is on another netns. >> If the returned id is NETNSA_NSID_NOT_ASSIGNED, then the netns is the same. >> >> Some lines before your patch, we call peernet_has_id() when the netns differ, >> thus we ensure that the id is available. > > Right, but that's internal to the kernel. Sure, but a good example exists in iproute2: https://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git/tree/ip/ipmonitor.c#n45 > >> The principle was that netlink messages of other netns can be sent only if an id >> is assigned. > > OK, could you please update include/uapi/linux/net_namespace.h to reflect that? > It says NETNSA_NSID_NOT_ASSIGNED are attributes for RTM_NEWNSID or RTM_GETNSID > which makes sense, but NOT_ASSIGNED sounds little like SAME_NSID for other > message types. I agree, it's confusing. I will send a patch.
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index ee841f0..b9f1392 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1414,8 +1414,10 @@ static void do_one_broadcast(struct sock *sk, p->skb2 = NULL; goto out; } - NETLINK_CB(p->skb2).nsid = peernet2id(sock_net(sk), p->net); - NETLINK_CB(p->skb2).nsid_is_set = true; + if (!net_eq(sock_net(sk), p->net)) { + NETLINK_CB(p->skb2).nsid = peernet2id(sock_net(sk), p->net); + NETLINK_CB(p->skb2).nsid_is_set = true; + } val = netlink_broadcast_deliver(sk, p->skb2); if (val < 0) { netlink_overrun(sk);
Don't include netns id for notifications broadcasts when the socket and the skb are in the same netns because it will be an error which can't be distinguished from a peer netns failing to allocate an id. Signed-off-by: Flavio Leitner <fbl@sysclose.org> --- net/netlink/af_netlink.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)