[PATCH v1 net 0/4] net: Support STP on bridge in non-root netns.

[PATCH v1 net 0/4] net: Support STP on bridge in non-root netns.

[Kuniyuki Iwashima]

Currently, STP does not work in non-root netns as llc_rcv() drops
packets from non-root netns.

This series fixes it by making some protocol handlers netns-aware,
which are called from llc_rcv() as follows:

  llc_rcv()
  |
  |- sap->rcv_func : registered by llc_sap_open()
  |
  |  * functions : regsitered by register_8022_client()
  |    -> No in-kernel user call register_8022_client()
  |
  |  * snap_rcv()
  |    |
  |    `- proto->rcvfunc() : registered by register_snap_client()
  |
  |       * aarp_rcv()  : drop packets from non-root netns
  |       * atalk_rcv() : drop packets from non-root netns
  |
  |  * stp_pdu_rcv()
  |    |
  |    `- garp_protos[]->rcv() : registered by stp_proto_register()
  |
  |       * garp_pdu_rcv() : netns-aware
  |       * br_stp_rcv()   : netns-aware
  |
  |- llc_type_handlers[llc_pdu_type(skb) - 1]
  |
  |  * llc_sap_handler()  : NOT netns-aware (Patch 1)
  |  * llc_conn_handler() : NOT netns-aware (Patch 2)
  |
  `- llc_station_handler

     * llc_station_rcv() : netns-aware

Patch 1 & 2 convert not-netns-aware functions and Patch 3 remove the
netns restriction in llc_rcv().

Note this series does not namespacify AF_LLC so that these patches
can be backported to stable tree (at least to 4.14.y) without conflicts.

Another series that adds netns support for AF_LLC will be targeted
to net-next later.


Kuniyuki Iwashima (4):
  llc: Check netns in llc_dgram_match().
  llc: Check netns in llc_estab_match() and llc_listener_match().
  llc: Don't drop packet from non-root netns.
  Revert "bridge: Add extack warning when enabling STP in netns."

 include/net/llc_conn.h |  2 +-
 net/bridge/br_stp_if.c |  3 ---
 net/llc/af_llc.c       |  2 +-
 net/llc/llc_conn.c     | 47 +++++++++++++++++++++++++-----------------
 net/llc/llc_if.c       |  2 +-
 net/llc/llc_input.c    |  3 ---
 net/llc/llc_sap.c      | 17 ++++++++-------
 7 files changed, 41 insertions(+), 35 deletions(-)

-- 
2.30.2

[PATCH v1 net 1/4] llc: Check netns in llc_dgram_match().

[Kuniyuki Iwashima]

We will remove this restriction in llc_rcv() in the following patch,
which means that the protocol handler must be aware of netns.

	if (!net_eq(dev_net(dev), &init_net))
		goto drop;

llc_rcv() fetches llc_type_handlers[llc_pdu_type(skb) - 1] and calls it
if not NULL.

If the PDU type is LLC_DEST_SAP, llc_sap_handler() is called to pass skb
to corresponding sockets.  Then, we must look up a proper socket in the
same netns with skb->dev.

If the destination is a multicast address, llc_sap_handler() calls
llc_sap_mcast().  It calculates a hash based on DSAP and skb->dev->ifindex,
iterates on a socket list, and calls llc_mcast_match() to check if the
socket is the correct destination.  Then, llc_mcast_match() checks if
skb->dev matches with llc_sk(sk)->dev.  So, we need not check netns here.

OTOH, if the destination is a unicast address, llc_sap_handler() calls
llc_lookup_dgram() to look up a socket, but it does not check the netns.

Therefore, we need to add netns check in llc_lookup_dgram().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/llc/llc_sap.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/llc/llc_sap.c b/net/llc/llc_sap.c
index 6805ce43a055..5c83fca3acd5 100644
--- a/net/llc/llc_sap.c
+++ b/net/llc/llc_sap.c
@@ -294,13 +294,15 @@ static void llc_sap_rcv(struct llc_sap *sap, struct sk_buff *skb,
 
 static inline bool llc_dgram_match(const struct llc_sap *sap,
 				   const struct llc_addr *laddr,
-				   const struct sock *sk)
+				   const struct sock *sk,
+				   const struct net *net)
 {
      struct llc_sock *llc = llc_sk(sk);
 
      return sk->sk_type == SOCK_DGRAM &&
-	  llc->laddr.lsap == laddr->lsap &&
-	  ether_addr_equal(llc->laddr.mac, laddr->mac);
+	     net_eq(sock_net(sk), net) &&
+	     llc->laddr.lsap == laddr->lsap &&
+	     ether_addr_equal(llc->laddr.mac, laddr->mac);
 }
 
 /**
@@ -312,7 +314,8 @@ static inline bool llc_dgram_match(const struct llc_sap *sap,
  *	mac, and local sap. Returns pointer for socket found, %NULL otherwise.
  */
 static struct sock *llc_lookup_dgram(struct llc_sap *sap,
-				     const struct llc_addr *laddr)
+				     const struct llc_addr *laddr,
+				     const struct net *net)
 {
 	struct sock *rc;
 	struct hlist_nulls_node *node;
@@ -322,12 +325,12 @@ static struct sock *llc_lookup_dgram(struct llc_sap *sap,
 	rcu_read_lock_bh();
 again:
 	sk_nulls_for_each_rcu(rc, node, laddr_hb) {
-		if (llc_dgram_match(sap, laddr, rc)) {
+		if (llc_dgram_match(sap, laddr, rc, net)) {
 			/* Extra checks required by SLAB_TYPESAFE_BY_RCU */
 			if (unlikely(!refcount_inc_not_zero(&rc->sk_refcnt)))
 				goto again;
 			if (unlikely(llc_sk(rc)->sap != sap ||
-				     !llc_dgram_match(sap, laddr, rc))) {
+				     !llc_dgram_match(sap, laddr, rc, net))) {
 				sock_put(rc);
 				continue;
 			}
@@ -429,7 +432,7 @@ void llc_sap_handler(struct llc_sap *sap, struct sk_buff *skb)
 		llc_sap_mcast(sap, &laddr, skb);
 		kfree_skb(skb);
 	} else {
-		struct sock *sk = llc_lookup_dgram(sap, &laddr);
+		struct sock *sk = llc_lookup_dgram(sap, &laddr, dev_net(skb->dev));
 		if (sk) {
 			llc_sap_rcv(sap, skb, sk);
 			sock_put(sk);
-- 
2.30.2

[PATCH v1 net 2/4] llc: Check netns in llc_estab_match() and llc_listener_match().

[Kuniyuki Iwashima]

We will remove this restriction in llc_rcv() in the following patch,
which means that the protocol handler must be aware of netns.

        if (!net_eq(dev_net(dev), &init_net))
                goto drop;

llc_rcv() fetches llc_type_handlers[llc_pdu_type(skb) - 1] and calls it
if not NULL.

If the PDU type is LLC_DEST_CONN, llc_conn_handler() is called to pass
skb to corresponding sockets.  Then, we must look up a proper socket in
the same netns with skb->dev.

llc_conn_handler() calls __llc_lookup() to look up a established or
litening socket by __llc_lookup_established() and llc_lookup_listener().

Both functions iterate on a list and call llc_estab_match() or
llc_listener_match() to check if the socket is the correct destination.
However, these functions do not check netns.

Also, bind() and connect() call llc_establish_connection(), which
finally calls __llc_lookup_established(), to check if there is a
conflicting socket.

Let's test netns in llc_estab_match() and llc_listener_match().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/llc_conn.h |  2 +-
 net/llc/af_llc.c       |  2 +-
 net/llc/llc_conn.c     | 47 +++++++++++++++++++++++++-----------------
 net/llc/llc_if.c       |  2 +-
 4 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/include/net/llc_conn.h b/include/net/llc_conn.h
index 2c1ea3414640..374411b3066c 100644
--- a/include/net/llc_conn.h
+++ b/include/net/llc_conn.h
@@ -111,7 +111,7 @@ void llc_conn_resend_i_pdu_as_cmd(struct sock *sk, u8 nr, u8 first_p_bit);
 void llc_conn_resend_i_pdu_as_rsp(struct sock *sk, u8 nr, u8 first_f_bit);
 int llc_conn_remove_acked_pdus(struct sock *conn, u8 nr, u16 *how_many_unacked);
 struct sock *llc_lookup_established(struct llc_sap *sap, struct llc_addr *daddr,
-				    struct llc_addr *laddr);
+				    struct llc_addr *laddr, const struct net *net);
 void llc_sap_add_socket(struct llc_sap *sap, struct sock *sk);
 void llc_sap_remove_socket(struct llc_sap *sap, struct sock *sk);
 
diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index 57c35c960b2c..9b06c380866b 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -402,7 +402,7 @@ static int llc_ui_bind(struct socket *sock, struct sockaddr *uaddr, int addrlen)
 		memcpy(laddr.mac, addr->sllc_mac, IFHWADDRLEN);
 		laddr.lsap = addr->sllc_sap;
 		rc = -EADDRINUSE; /* mac + sap clash. */
-		ask = llc_lookup_established(sap, &daddr, &laddr);
+		ask = llc_lookup_established(sap, &daddr, &laddr, &init_net);
 		if (ask) {
 			sock_put(ask);
 			goto out_put;
diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 912aa9bd5e29..6371c1b903d4 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -453,11 +453,13 @@ static int llc_exec_conn_trans_actions(struct sock *sk,
 static inline bool llc_estab_match(const struct llc_sap *sap,
 				   const struct llc_addr *daddr,
 				   const struct llc_addr *laddr,
-				   const struct sock *sk)
+				   const struct sock *sk,
+				   const struct net *net)
 {
 	struct llc_sock *llc = llc_sk(sk);
 
-	return llc->laddr.lsap == laddr->lsap &&
+	return net_eq(sock_net(sk), net) &&
+		llc->laddr.lsap == laddr->lsap &&
 		llc->daddr.lsap == daddr->lsap &&
 		ether_addr_equal(llc->laddr.mac, laddr->mac) &&
 		ether_addr_equal(llc->daddr.mac, daddr->mac);
@@ -476,7 +478,8 @@ static inline bool llc_estab_match(const struct llc_sap *sap,
  */
 static struct sock *__llc_lookup_established(struct llc_sap *sap,
 					     struct llc_addr *daddr,
-					     struct llc_addr *laddr)
+					     struct llc_addr *laddr,
+					     const struct net *net)
 {
 	struct sock *rc;
 	struct hlist_nulls_node *node;
@@ -486,12 +489,12 @@ static struct sock *__llc_lookup_established(struct llc_sap *sap,
 	rcu_read_lock();
 again:
 	sk_nulls_for_each_rcu(rc, node, laddr_hb) {
-		if (llc_estab_match(sap, daddr, laddr, rc)) {
+		if (llc_estab_match(sap, daddr, laddr, rc, net)) {
 			/* Extra checks required by SLAB_TYPESAFE_BY_RCU */
 			if (unlikely(!refcount_inc_not_zero(&rc->sk_refcnt)))
 				goto again;
 			if (unlikely(llc_sk(rc)->sap != sap ||
-				     !llc_estab_match(sap, daddr, laddr, rc))) {
+				     !llc_estab_match(sap, daddr, laddr, rc, net))) {
 				sock_put(rc);
 				continue;
 			}
@@ -513,29 +516,33 @@ static struct sock *__llc_lookup_established(struct llc_sap *sap,
 
 struct sock *llc_lookup_established(struct llc_sap *sap,
 				    struct llc_addr *daddr,
-				    struct llc_addr *laddr)
+				    struct llc_addr *laddr,
+				    const struct net *net)
 {
 	struct sock *sk;
 
 	local_bh_disable();
-	sk = __llc_lookup_established(sap, daddr, laddr);
+	sk = __llc_lookup_established(sap, daddr, laddr, net);
 	local_bh_enable();
 	return sk;
 }
 
 static inline bool llc_listener_match(const struct llc_sap *sap,
 				      const struct llc_addr *laddr,
-				      const struct sock *sk)
+				      const struct sock *sk,
+				      const struct net *net)
 {
 	struct llc_sock *llc = llc_sk(sk);
 
-	return sk->sk_type == SOCK_STREAM && sk->sk_state == TCP_LISTEN &&
+	return net_eq(sock_net(sk), net) &&
+		sk->sk_type == SOCK_STREAM && sk->sk_state == TCP_LISTEN &&
 		llc->laddr.lsap == laddr->lsap &&
 		ether_addr_equal(llc->laddr.mac, laddr->mac);
 }
 
 static struct sock *__llc_lookup_listener(struct llc_sap *sap,
-					  struct llc_addr *laddr)
+					  struct llc_addr *laddr,
+					  const struct net *net)
 {
 	struct sock *rc;
 	struct hlist_nulls_node *node;
@@ -545,12 +552,12 @@ static struct sock *__llc_lookup_listener(struct llc_sap *sap,
 	rcu_read_lock();
 again:
 	sk_nulls_for_each_rcu(rc, node, laddr_hb) {
-		if (llc_listener_match(sap, laddr, rc)) {
+		if (llc_listener_match(sap, laddr, rc, net)) {
 			/* Extra checks required by SLAB_TYPESAFE_BY_RCU */
 			if (unlikely(!refcount_inc_not_zero(&rc->sk_refcnt)))
 				goto again;
 			if (unlikely(llc_sk(rc)->sap != sap ||
-				     !llc_listener_match(sap, laddr, rc))) {
+				     !llc_listener_match(sap, laddr, rc, net))) {
 				sock_put(rc);
 				continue;
 			}
@@ -581,24 +588,26 @@ static struct sock *__llc_lookup_listener(struct llc_sap *sap,
  *	Caller has to make sure local_bh is disabled.
  */
 static struct sock *llc_lookup_listener(struct llc_sap *sap,
-					struct llc_addr *laddr)
+					struct llc_addr *laddr,
+					const struct net *net)
 {
+	struct sock *rc = __llc_lookup_listener(sap, laddr, net);
 	static struct llc_addr null_addr;
-	struct sock *rc = __llc_lookup_listener(sap, laddr);
 
 	if (!rc)
-		rc = __llc_lookup_listener(sap, &null_addr);
+		rc = __llc_lookup_listener(sap, &null_addr, net);
 
 	return rc;
 }
 
 static struct sock *__llc_lookup(struct llc_sap *sap,
 				 struct llc_addr *daddr,
-				 struct llc_addr *laddr)
+				 struct llc_addr *laddr,
+				 const struct net *net)
 {
-	struct sock *sk = __llc_lookup_established(sap, daddr, laddr);
+	struct sock *sk = __llc_lookup_established(sap, daddr, laddr, net);
 
-	return sk ? : llc_lookup_listener(sap, laddr);
+	return sk ? : llc_lookup_listener(sap, laddr, net);
 }
 
 /**
@@ -776,7 +785,7 @@ void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb)
 	llc_pdu_decode_da(skb, daddr.mac);
 	llc_pdu_decode_dsap(skb, &daddr.lsap);
 
-	sk = __llc_lookup(sap, &saddr, &daddr);
+	sk = __llc_lookup(sap, &saddr, &daddr, dev_net(skb->dev));
 	if (!sk)
 		goto drop;
 
diff --git a/net/llc/llc_if.c b/net/llc/llc_if.c
index dde9bf08a593..58a5f419adc6 100644
--- a/net/llc/llc_if.c
+++ b/net/llc/llc_if.c
@@ -92,7 +92,7 @@ int llc_establish_connection(struct sock *sk, const u8 *lmac, u8 *dmac, u8 dsap)
 	daddr.lsap = dsap;
 	memcpy(daddr.mac, dmac, sizeof(daddr.mac));
 	memcpy(laddr.mac, lmac, sizeof(laddr.mac));
-	existing = llc_lookup_established(llc->sap, &daddr, &laddr);
+	existing = llc_lookup_established(llc->sap, &daddr, &laddr, sock_net(sk));
 	if (existing) {
 		if (existing->sk_state == TCP_ESTABLISHED) {
 			sk = existing;
-- 
2.30.2

[PATCH v1 net 3/4] llc: Don't drop packet from non-root netns.

[Kuniyuki Iwashima]

Now these upper layer protocol handlers can be called from llc_rcv()
as sap->rcv_func(), which is registered by llc_sap_open().

  * function which is passed to register_8022_client()
    -> no in-kernel user calls register_8022_client().

  * snap_rcv()
    `- proto->rcvfunc() : registered by register_snap_client()
       -> aarp_rcv() and atalk_rcv() drop packets from non-root netns

  * stp_pdu_rcv()
    `- garp_protos[]->rcv() : registered by stp_proto_register()
       -> garp_pdu_rcv() and br_stp_rcv() are netns-aware

So, we can safely remove the netns restriction in llc_rcv().

Fixes: e730c15519d0 ("[NET]: Make packet reception network namespace safe")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/llc/llc_input.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/llc/llc_input.c b/net/llc/llc_input.c
index c309b72a5877..7cac441862e2 100644
--- a/net/llc/llc_input.c
+++ b/net/llc/llc_input.c
@@ -163,9 +163,6 @@ int llc_rcv(struct sk_buff *skb, struct net_device *dev,
 	void (*sta_handler)(struct sk_buff *skb);
 	void (*sap_handler)(struct llc_sap *sap, struct sk_buff *skb);
 
-	if (!net_eq(dev_net(dev), &init_net))
-		goto drop;
-
 	/*
 	 * When the interface is in promisc. mode, drop all the crap that it
 	 * receives, do not try to analyse it.
-- 
2.30.2

[PATCH v1 net 4/4] Revert "bridge: Add extack warning when enabling STP in netns."

[Kuniyuki Iwashima]

This reverts commit 56a16035bb6effb37177867cea94c13a8382f745.

Since the previous commit, STP works on bridge in netns.

  # unshare -n
  # ip link add br0 type bridge
  # ip link add veth0 type veth peer name veth1

  # ip link set veth0 master br0 up
  [   50.558135] br0: port 1(veth0) entered blocking state
  [   50.558366] br0: port 1(veth0) entered disabled state
  [   50.558798] veth0: entered allmulticast mode
  [   50.564401] veth0: entered promiscuous mode

  # ip link set veth1 master br0 up
  [   54.215487] br0: port 2(veth1) entered blocking state
  [   54.215657] br0: port 2(veth1) entered disabled state
  [   54.215848] veth1: entered allmulticast mode
  [   54.219577] veth1: entered promiscuous mode

  # ip link set br0 type bridge stp_state 1
  # ip link set br0 up
  [   61.960726] br0: port 2(veth1) entered blocking state
  [   61.961097] br0: port 2(veth1) entered listening state
  [   61.961495] br0: port 1(veth0) entered blocking state
  [   61.961653] br0: port 1(veth0) entered listening state
  [   63.998835] br0: port 2(veth1) entered blocking state
  [   77.437113] br0: port 1(veth0) entered learning state
  [   86.653501] br0: received packet on veth0 with own address as source address (addr:6e:0f:e7:6f:5f:5f, vlan:0)
  [   92.797095] br0: port 1(veth0) entered forwarding state
  [   92.797398] br0: topology change detected, propagating

Let's remove the warning.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/bridge/br_stp_if.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index b65962682771..75204d36d7f9 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -201,9 +201,6 @@ int br_stp_set_enabled(struct net_bridge *br, unsigned long val,
 {
 	ASSERT_RTNL();
 
-	if (!net_eq(dev_net(br->dev), &init_net))
-		NL_SET_ERR_MSG_MOD(extack, "STP does not work in non-root netns");
-
 	if (br_mrp_enabled(br)) {
 		NL_SET_ERR_MSG_MOD(extack,
 				   "STP can't be enabled if MRP is already enabled");
-- 
2.30.2

Re: [PATCH v1 net 1/4] llc: Check netns in llc_dgram_match().

[Paolo Abeni]

Hi,

the series LGTM, I have only a couple of (very) minor nit, see below...

On Fri, 2023-07-14 at 19:13 -0700, Kuniyuki Iwashima wrote:
> We will remove this restriction in llc_rcv() in the following patch,

s/following patch/soon/, as the following patch will not do that ;)

> which means that the protocol handler must be aware of netns.
> 
> 	if (!net_eq(dev_net(dev), &init_net))
> 		goto drop;
> 
> llc_rcv() fetches llc_type_handlers[llc_pdu_type(skb) - 1] and calls it
> if not NULL.
> 
> If the PDU type is LLC_DEST_SAP, llc_sap_handler() is called to pass skb
> to corresponding sockets.  Then, we must look up a proper socket in the
> same netns with skb->dev.
> 
> If the destination is a multicast address, llc_sap_handler() calls
> llc_sap_mcast().  It calculates a hash based on DSAP and skb->dev->ifindex,
> iterates on a socket list, and calls llc_mcast_match() to check if the
> socket is the correct destination.  Then, llc_mcast_match() checks if
> skb->dev matches with llc_sk(sk)->dev.  So, we need not check netns here.
> 
> OTOH, if the destination is a unicast address, llc_sap_handler() calls
> llc_lookup_dgram() to look up a socket, but it does not check the netns.
> 
> Therefore, we need to add netns check in llc_lookup_dgram().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/llc/llc_sap.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/net/llc/llc_sap.c b/net/llc/llc_sap.c
> index 6805ce43a055..5c83fca3acd5 100644
> --- a/net/llc/llc_sap.c
> +++ b/net/llc/llc_sap.c
> @@ -294,13 +294,15 @@ static void llc_sap_rcv(struct llc_sap *sap, struct sk_buff *skb,
>  
>  static inline bool llc_dgram_match(const struct llc_sap *sap,
>  				   const struct llc_addr *laddr,
> -				   const struct sock *sk)
> +				   const struct sock *sk,
> +				   const struct net *net)
>  {
>       struct llc_sock *llc = llc_sk(sk);
>  
>       return sk->sk_type == SOCK_DGRAM &&
> -	  llc->laddr.lsap == laddr->lsap &&
> -	  ether_addr_equal(llc->laddr.mac, laddr->mac);
> +	     net_eq(sock_net(sk), net) &&
> +	     llc->laddr.lsap == laddr->lsap &&
> +	     ether_addr_equal(llc->laddr.mac, laddr->mac);
>  }
>  
>  /**
> @@ -312,7 +314,8 @@ static inline bool llc_dgram_match(const struct llc_sap *sap,
>   *	mac, and local sap. Returns pointer for socket found, %NULL otherwise.
>   */

As this function has doxygen documentation, you should additionally
document the 'net' argument.

Thanks,

Paolo

Re: [PATCH v1 net 2/4] llc: Check netns in llc_estab_match() and llc_listener_match().

[Paolo Abeni]

On Fri, 2023-07-14 at 19:13 -0700, Kuniyuki Iwashima wrote:
> @@ -476,7 +478,8 @@ static inline bool llc_estab_match(const struct llc_sap *sap,
>   */
>  static struct sock *__llc_lookup_established(struct llc_sap *sap,
>  					     struct llc_addr *daddr,
> -					     struct llc_addr *laddr)
> +					     struct llc_addr *laddr,
> +					     const struct net *net)

You should add 'net' to the doxygen documentation.

>  {
>  	struct sock *rc;
>  	struct hlist_nulls_node *node;

[...]

> @@ -581,24 +588,26 @@ static struct sock *__llc_lookup_listener(struct llc_sap *sap,
>   *	Caller has to make sure local_bh is disabled.
>   */
>  static struct sock *llc_lookup_listener(struct llc_sap *sap,
> -					struct llc_addr *laddr)
> +					struct llc_addr *laddr,
> +					const struct net *net)

Same here.

Thanks!

Paolo

Re: [PATCH v1 net 1/4] llc: Check netns in llc_dgram_match().

[Kuniyuki Iwashima]

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 18 Jul 2023 12:34:47 +0200
> Hi,
> 
> the series LGTM, I have only a couple of (very) minor nit, see below...
> 
> On Fri, 2023-07-14 at 19:13 -0700, Kuniyuki Iwashima wrote:
> > We will remove this restriction in llc_rcv() in the following patch,
> 
> s/following patch/soon/, as the following patch will not do that ;)

Exactly, will fix :)


> 
> > which means that the protocol handler must be aware of netns.
> > 
> > 	if (!net_eq(dev_net(dev), &init_net))
> > 		goto drop;
> > 
> > llc_rcv() fetches llc_type_handlers[llc_pdu_type(skb) - 1] and calls it
> > if not NULL.
> > 
> > If the PDU type is LLC_DEST_SAP, llc_sap_handler() is called to pass skb
> > to corresponding sockets.  Then, we must look up a proper socket in the
> > same netns with skb->dev.
> > 
> > If the destination is a multicast address, llc_sap_handler() calls
> > llc_sap_mcast().  It calculates a hash based on DSAP and skb->dev->ifindex,
> > iterates on a socket list, and calls llc_mcast_match() to check if the
> > socket is the correct destination.  Then, llc_mcast_match() checks if
> > skb->dev matches with llc_sk(sk)->dev.  So, we need not check netns here.
> > 
> > OTOH, if the destination is a unicast address, llc_sap_handler() calls
> > llc_lookup_dgram() to look up a socket, but it does not check the netns.
> > 
> > Therefore, we need to add netns check in llc_lookup_dgram().
> > 
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >  net/llc/llc_sap.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/llc/llc_sap.c b/net/llc/llc_sap.c
> > index 6805ce43a055..5c83fca3acd5 100644
> > --- a/net/llc/llc_sap.c
> > +++ b/net/llc/llc_sap.c
> > @@ -294,13 +294,15 @@ static void llc_sap_rcv(struct llc_sap *sap, struct sk_buff *skb,
> >  
> >  static inline bool llc_dgram_match(const struct llc_sap *sap,
> >  				   const struct llc_addr *laddr,
> > -				   const struct sock *sk)
> > +				   const struct sock *sk,
> > +				   const struct net *net)
> >  {
> >       struct llc_sock *llc = llc_sk(sk);
> >  
> >       return sk->sk_type == SOCK_DGRAM &&
> > -	  llc->laddr.lsap == laddr->lsap &&
> > -	  ether_addr_equal(llc->laddr.mac, laddr->mac);
> > +	     net_eq(sock_net(sk), net) &&
> > +	     llc->laddr.lsap == laddr->lsap &&
> > +	     ether_addr_equal(llc->laddr.mac, laddr->mac);
> >  }
> >  
> >  /**
> > @@ -312,7 +314,8 @@ static inline bool llc_dgram_match(const struct llc_sap *sap,
> >   *	mac, and local sap. Returns pointer for socket found, %NULL otherwise.
> >   */
> 
> As this function has doxygen documentation, you should additionally
> document the 'net' argument.

And will fix this and the same miss in patch 2.

Thanks for catching this!


> 
> Thanks,
> 
> Paolo