* j1939: Attempt to make j1939 network namespace-aware
@ 2018-02-05 20:31 Elenita Hinds
2018-02-05 20:44 ` Kurt Van Dijck
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Elenita Hinds @ 2018-02-05 20:31 UTC (permalink / raw)
To: linux-can, dev.kurt
Hi all,
Below is a first attempt to make the j1939 kernel support be network
namespace-aware (needed for my project). Please review, especially
Kurt and Marc, as there maybe better ways of doing this. I'll
re-submit a more official patch after getting feedback/comments.
Please note the TODO in j1939/transport.c, j1939tp_module_init().
Regards,
Elenita Hinds
---
diff --git a/j1939/address-claim.c b/j1939/address-claim.c
index ab538e1c4906..784e91762c6e 100644
--- a/j1939/address-claim.c
+++ b/j1939/address-claim.c
@@ -89,8 +89,9 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
/* return both when failure & when successful */
if (ret < 0)
return ret;
- ecu = j1939_ecu_find_by_name(skcb->addr.src_name,
- skb->dev->ifindex);
+ ecu = j1939_ecu_find_by_name(dev_net(skb->dev),
+ skcb->addr.src_name,
+ skb->dev->ifindex);
if (!ecu)
return -ENODEV;
@@ -100,7 +101,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
put_j1939_ecu(ecu);
} else if (skcb->addr.src_name) {
/* assign source address */
- sa = j1939_name_to_sa(skcb->addr.src_name, skb->dev->ifindex);
+ sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.src_name,
+ skb->dev->ifindex);
if (!j1939_address_is_unicast(sa) &&
!ac_msg_is_request_for_ac(skb)) {
pr_notice("tx drop: invalid sa for name 0x%016llx\n",
@@ -112,7 +114,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
/* assign destination address */
if (skcb->addr.dst_name) {
- sa = j1939_name_to_sa(skcb->addr.dst_name, skb->dev->ifindex);
+ sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.dst_name,
+ skb->dev->ifindex);
if (!j1939_address_is_unicast(sa)) {
pr_notice("tx drop: invalid da for name 0x%016llx\n",
skcb->addr.dst_name);
@@ -123,7 +126,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
return 0;
}
-static void j1939_process_address_claim(struct sk_buff *skb, struct
j1939_priv *priv)
+static void j1939_process_address_claim(struct sk_buff *skb,
+ struct j1939_priv *priv)
{
struct j1939_sk_buff_cb *skcb = j1939_get_cb(skb);
struct j1939_ecu *ecu, *prev;
diff --git a/j1939/bus.c b/j1939/bus.c
index 9ba6da216c1b..1af93eb4d939 100644
--- a/j1939/bus.c
+++ b/j1939/bus.c
@@ -110,7 +110,7 @@ struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa,
struct j1939_priv *priv)
return ecu;
}
-u8 j1939_name_to_sa(name_t name, int ifindex)
+u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex)
{
struct j1939_ecu *ecu;
struct j1939_priv *priv;
@@ -118,7 +118,7 @@ u8 j1939_name_to_sa(name_t name, int ifindex)
if (!name)
return J1939_NO_ADDR;
- priv = j1939_priv_get_by_ifindex(ifindex);
+ priv = j1939_priv_get_by_ifindex(net, ifindex);
if (!priv)
return J1939_NO_ADDR;
@@ -157,7 +157,7 @@ static struct j1939_ecu
*_j1939_ecu_find_by_name(name_t name,
}
/* ecu lookup by name */
-struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex)
+struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
name, int ifindex)
{
struct j1939_ecu *ecu;
struct j1939_priv *priv;
@@ -166,7 +166,7 @@ struct j1939_ecu *j1939_ecu_find_by_name(name_t
name, int ifindex)
return NULL;
if (!ifindex)
return NULL;
- priv = j1939_priv_get_by_ifindex(ifindex);
+ priv = j1939_priv_get_by_ifindex(net, ifindex);
if (!priv)
return NULL;
ecu = _j1939_ecu_find_by_name(name, priv);
diff --git a/j1939/j1939-priv.h b/j1939/j1939-priv.h
index 2e54092e946a..95288dbc5a90 100644
--- a/j1939/j1939-priv.h
+++ b/j1939/j1939-priv.h
@@ -174,9 +174,9 @@ static inline void j1939_ecu_remove_sa(struct
j1939_ecu *ecu)
write_unlock_bh(&ecu->priv->lock);
}
-u8 j1939_name_to_sa(name_t name, int ifindex);
+u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex);
struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa, struct j1939_priv *priv);
-struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex);
+struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
name, int ifindex);
/* find_by_name, with kref & read_lock taken */
struct j1939_ecu *j1939_ecu_find_priv_default_tx(int ifindex, name_t *pname,
u8 *paddr);
@@ -245,12 +245,12 @@ struct j1939_ecu *_j1939_ecu_get_register(struct
j1939_priv *priv,
/* unregister must be called with lock held */
void _j1939_ecu_unregister(struct j1939_ecu *);
-int j1939_netdev_start(struct net_device *);
-void j1939_netdev_stop(struct net_device *);
+int j1939_netdev_start(struct net *net, struct net_device *);
+void j1939_netdev_stop(struct net *net, struct net_device *);
void __j1939_priv_release(struct kref *kref);
struct j1939_priv *j1939_priv_get(struct net_device *dev);
-struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex);
+struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int ifindex);
static inline void j1939_priv_set(struct net_device *dev, struct
j1939_priv *priv)
diff --git a/j1939/main.c b/j1939/main.c
index 736e9ac8090c..0e55569fb1e6 100644
--- a/j1939/main.c
+++ b/j1939/main.c
@@ -196,7 +196,7 @@ static void j1939_priv_ac_task(unsigned long val)
static DEFINE_SPINLOCK(j1939_netdev_lock);
-int j1939_netdev_start(struct net_device *netdev)
+int j1939_netdev_start(struct net *net, struct net_device *netdev)
{
struct j1939_priv *priv;
int ret;
@@ -221,7 +221,7 @@ int j1939_netdev_start(struct net_device *netdev)
dev_hold(netdev);
/* add CAN handler */
- ret = can_rx_register(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
+ ret = can_rx_register(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
j1939_can_recv, priv, "j1939", NULL);
if (ret < 0)
goto out_dev_put;
@@ -229,7 +229,8 @@ int j1939_netdev_start(struct net_device *netdev)
spin_lock(&j1939_netdev_lock);
if (j1939_priv_get(netdev)) {
/* Someone was faster than us, use their priv and roll
- * back our's. */
+ * back our's.
+ */
spin_unlock(&j1939_netdev_lock);
goto out_rx_unregister;
}
@@ -239,7 +240,7 @@ int j1939_netdev_start(struct net_device *netdev)
return 0;
out_rx_unregister:
- can_rx_unregister(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
+ can_rx_unregister(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
j1939_can_recv, priv);
out_dev_put:
dev_put(netdev);
@@ -248,12 +249,14 @@ int j1939_netdev_start(struct net_device *netdev)
return ret;
}
-void j1939_netdev_stop(struct net_device *netdev)
+void j1939_netdev_stop(struct net *net, struct net_device *netdev)
{
struct j1939_priv *priv;
spin_lock(&j1939_netdev_lock);
priv = __j1939_priv_get(netdev);
+ can_rx_unregister(net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
+ j1939_can_recv, priv);
j1939_priv_put(priv);
spin_unlock(&j1939_netdev_lock);
}
@@ -264,9 +267,6 @@ void __j1939_priv_release(struct kref *kref)
struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
struct j1939_ecu *ecu;
- can_rx_unregister(&init_net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
- j1939_can_recv, priv);
-
tasklet_disable_nosync(&priv->ac_task);
/* remove pending transport protocol sessions */
@@ -302,12 +302,12 @@ struct j1939_priv *j1939_priv_get(struct net_device *dev)
return priv;
}
-struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex)
+struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int ifindex)
{
struct j1939_priv *priv;
struct net_device *netdev;
- netdev = dev_get_by_index(&init_net, ifindex);
+ netdev = dev_get_by_index(net, ifindex);
if (!netdev)
return NULL;
@@ -321,8 +321,9 @@ static int j1939_netdev_notify(struct notifier_block *nb,
unsigned long msg, void *data)
{
struct net_device *netdev = netdev_notifier_info_to_dev(data);
+ struct net *net = dev_net(netdev);
- if (!net_eq(dev_net(netdev), &init_net))
+ if (!net_eq(dev_net(netdev), net))
return NOTIFY_DONE;
if (netdev->type != ARPHRD_CAN)
diff --git a/j1939/socket.c b/j1939/socket.c
index ae5357510623..fe70411cdf88 100644
--- a/j1939/socket.c
+++ b/j1939/socket.c
@@ -200,6 +200,7 @@ static int j1939sk_init(struct sock *sk)
static int j1939sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
{
struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
+ struct sock *sk = sock->sk;
struct j1939_sock *jsk = j1939_sk(sock->sk);
struct net_device *netdev;
struct j1939_priv *priv;
@@ -219,7 +220,7 @@ static int j1939sk_bind(struct socket *sock,
struct sockaddr *uaddr, int len)
lock_sock(sock->sk);
- netdev = dev_get_by_index(&init_net, addr->can_ifindex);
+ netdev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
if (!netdev) {
ret = -ENODEV;
goto out_release_sock;
@@ -245,8 +246,8 @@ static int j1939sk_bind(struct socket *sock,
struct sockaddr *uaddr, int len)
goto out_dev_put;
}
- ret = j1939_netdev_start(netdev);
- if (ret < 0)
+ ret = j1939_netdev_start(sock_net(sk), netdev);
+ if (ret < 0)
goto out_dev_put;
jsk->sk.sk_bound_dev_if = addr->can_ifindex;
@@ -384,14 +385,14 @@ static int j1939sk_release(struct socket *sock)
list_del_init(&jsk->list);
spin_unlock_bh(&j1939_socks_lock);
- netdev = dev_get_by_index(&init_net, jsk->sk.sk_bound_dev_if);
+ netdev = dev_get_by_index(sock_net(sk), jsk->sk.sk_bound_dev_if);
if (netdev) {
priv = j1939_priv_get(netdev);
j1939_addr_local_put(priv, jsk->addr.sa);
j1939_name_local_put(priv, jsk->addr.src_name);
j1939_priv_put(priv);
- j1939_netdev_stop(netdev);
+ j1939_netdev_stop(sock_net(sk), netdev);
dev_put(netdev);
}
}
@@ -457,9 +458,11 @@ static int j1939sk_setsockopt(struct socket
*sock, int level, int optname,
kfree(ofilters);
return 0;
case SO_J1939_PROMISC:
- return j1939sk_setsockopt_flag(jsk, optval, optlen,
J1939_SOCK_PROMISC);
+ return j1939sk_setsockopt_flag(jsk, optval, optlen,
+ J1939_SOCK_PROMISC);
case SO_J1939_RECV_OWN:
- return j1939sk_setsockopt_flag(jsk, optval, optlen,
J1939_SOCK_RECV_OWN);
+ return j1939sk_setsockopt_flag(jsk, optval, optlen,
+ J1939_SOCK_RECV_OWN);
case SO_J1939_SEND_PRIO:
if (optlen != sizeof(tmp))
return -EINVAL;
@@ -618,7 +621,7 @@ static int j1939sk_sendmsg(struct socket *sock,
struct msghdr *msg, size_t size)
return -EBADFD;
}
- dev = dev_get_by_index(&init_net, ifindex);
+ dev = dev_get_by_index(sock_net(sk), ifindex);
if (!dev)
return -ENXIO;
@@ -714,7 +717,7 @@ void j1939sk_netdev_event(struct net_device
*netdev, int error_code)
j1939_name_local_put(priv, jsk->addr.src_name);
j1939_priv_put(priv);
- j1939_netdev_stop(netdev);
+ j1939_netdev_stop(sock_net(&jsk->sk), netdev);
}
/* do not remove filters here */
}
diff --git a/j1939/transport.c b/j1939/transport.c
index fbebcc20bccf..67722773c0b0 100644
--- a/j1939/transport.c
+++ b/j1939/transport.c
@@ -107,8 +107,8 @@ struct session {
/* forward declarations */
static struct session *j1939_session_new(struct sk_buff *skb);
static struct session *j1939_session_fresh_new(int size,
- struct sk_buff *rel_skb,
- pgn_t pgn);
+ struct sk_buff *rel_skb,
+ pgn_t pgn);
static void j1939tp_del_work(struct work_struct *work);
/* local variables */
@@ -1152,7 +1152,7 @@ int j1939_send_transport(struct sk_buff *skb)
return ret;
/* fix dst_flags, it may be used there soon */
- priv = j1939_priv_get_by_ifindex(can_skb_prv(skb)->ifindex);
+ priv = j1939_priv_get_by_ifindex(dev_net(skb->dev),
can_skb_prv(skb)->ifindex);
if (!priv)
return -EINVAL;
if (j1939_address_is_unicast(cb->addr.da) &&
@@ -1273,8 +1273,8 @@ int j1939_recv_transport(struct sk_buff *skb)
}
static struct session *j1939_session_fresh_new(int size,
- struct sk_buff *rel_skb,
- pgn_t pgn)
+ struct sk_buff *rel_skb,
+ pgn_t pgn)
{
struct sk_buff *skb;
struct j1939_sk_buff_cb *cb;
@@ -1400,6 +1400,7 @@ static struct ctl_table_header *sysctl_hdr;
/* module init */
int __init j1939tp_module_init(void)
{
+ //TODO: not sure how to make this network namespace-aware??
sysctl_hdr = register_net_sysctl(&init_net, "net/can-j1939",
canj1939_sysctl_table);
if (!sysctl_hdr)
---
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-05 20:31 j1939: Attempt to make j1939 network namespace-aware Elenita Hinds
@ 2018-02-05 20:44 ` Kurt Van Dijck
2018-02-06 16:15 ` Elenita Hinds
2018-02-06 8:52 ` Robert Schwebel
2018-02-06 20:37 ` Kurt Van Dijck
2 siblings, 1 reply; 12+ messages in thread
From: Kurt Van Dijck @ 2018-02-05 20:44 UTC (permalink / raw)
To: Elenita Hinds, linux-can
Hey,
I'm not so used to containers. Can you explain a bit what I should expect from network namespaces wrt. J1939? What is unique, what will be duplicated, ...
That will make the review easier, as I will understand better the purpose than I do now.
Kurt
On 5 February 2018 21:31:32 CET, Elenita Hinds <ecathinds@gmail.com> wrote:
>Hi all,
>
>Below is a first attempt to make the j1939 kernel support be network
>namespace-aware (needed for my project). Please review, especially
>Kurt and Marc, as there maybe better ways of doing this. I'll
>re-submit a more official patch after getting feedback/comments.
>
>Please note the TODO in j1939/transport.c, j1939tp_module_init().
>
>Regards,
>Elenita Hinds
>
>---
>diff --git a/j1939/address-claim.c b/j1939/address-claim.c
>index ab538e1c4906..784e91762c6e 100644
>--- a/j1939/address-claim.c
>+++ b/j1939/address-claim.c
>@@ -89,8 +89,9 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> /* return both when failure & when successful */
> if (ret < 0)
> return ret;
>- ecu = j1939_ecu_find_by_name(skcb->addr.src_name,
>- skb->dev->ifindex);
>+ ecu = j1939_ecu_find_by_name(dev_net(skb->dev),
>+ skcb->addr.src_name,
>+ skb->dev->ifindex);
> if (!ecu)
> return -ENODEV;
>
>@@ -100,7 +101,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> put_j1939_ecu(ecu);
> } else if (skcb->addr.src_name) {
> /* assign source address */
>- sa = j1939_name_to_sa(skcb->addr.src_name, skb->dev->ifindex);
>+ sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.src_name,
>+ skb->dev->ifindex);
> if (!j1939_address_is_unicast(sa) &&
> !ac_msg_is_request_for_ac(skb)) {
> pr_notice("tx drop: invalid sa for name 0x%016llx\n",
>@@ -112,7 +114,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>
> /* assign destination address */
> if (skcb->addr.dst_name) {
>- sa = j1939_name_to_sa(skcb->addr.dst_name, skb->dev->ifindex);
>+ sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.dst_name,
>+ skb->dev->ifindex);
> if (!j1939_address_is_unicast(sa)) {
> pr_notice("tx drop: invalid da for name 0x%016llx\n",
> skcb->addr.dst_name);
>@@ -123,7 +126,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> return 0;
> }
>
>-static void j1939_process_address_claim(struct sk_buff *skb, struct
>j1939_priv *priv)
>+static void j1939_process_address_claim(struct sk_buff *skb,
>+ struct j1939_priv *priv)
> {
> struct j1939_sk_buff_cb *skcb = j1939_get_cb(skb);
> struct j1939_ecu *ecu, *prev;
>diff --git a/j1939/bus.c b/j1939/bus.c
>index 9ba6da216c1b..1af93eb4d939 100644
>--- a/j1939/bus.c
>+++ b/j1939/bus.c
>@@ -110,7 +110,7 @@ struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa,
>struct j1939_priv *priv)
> return ecu;
> }
>
>-u8 j1939_name_to_sa(name_t name, int ifindex)
>+u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex)
> {
> struct j1939_ecu *ecu;
> struct j1939_priv *priv;
>@@ -118,7 +118,7 @@ u8 j1939_name_to_sa(name_t name, int ifindex)
>
> if (!name)
> return J1939_NO_ADDR;
>- priv = j1939_priv_get_by_ifindex(ifindex);
>+ priv = j1939_priv_get_by_ifindex(net, ifindex);
> if (!priv)
> return J1939_NO_ADDR;
>
>@@ -157,7 +157,7 @@ static struct j1939_ecu
>*_j1939_ecu_find_by_name(name_t name,
> }
>
> /* ecu lookup by name */
>-struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex)
>+struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
>name, int ifindex)
> {
> struct j1939_ecu *ecu;
> struct j1939_priv *priv;
>@@ -166,7 +166,7 @@ struct j1939_ecu *j1939_ecu_find_by_name(name_t
>name, int ifindex)
> return NULL;
> if (!ifindex)
> return NULL;
>- priv = j1939_priv_get_by_ifindex(ifindex);
>+ priv = j1939_priv_get_by_ifindex(net, ifindex);
> if (!priv)
> return NULL;
> ecu = _j1939_ecu_find_by_name(name, priv);
>diff --git a/j1939/j1939-priv.h b/j1939/j1939-priv.h
>index 2e54092e946a..95288dbc5a90 100644
>--- a/j1939/j1939-priv.h
>+++ b/j1939/j1939-priv.h
>@@ -174,9 +174,9 @@ static inline void j1939_ecu_remove_sa(struct
>j1939_ecu *ecu)
> write_unlock_bh(&ecu->priv->lock);
> }
>
>-u8 j1939_name_to_sa(name_t name, int ifindex);
>+u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex);
>struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa, struct j1939_priv
>*priv);
>-struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex);
>+struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
>name, int ifindex);
> /* find_by_name, with kref & read_lock taken */
>struct j1939_ecu *j1939_ecu_find_priv_default_tx(int ifindex, name_t
>*pname,
> u8 *paddr);
>@@ -245,12 +245,12 @@ struct j1939_ecu *_j1939_ecu_get_register(struct
>j1939_priv *priv,
> /* unregister must be called with lock held */
> void _j1939_ecu_unregister(struct j1939_ecu *);
>
>-int j1939_netdev_start(struct net_device *);
>-void j1939_netdev_stop(struct net_device *);
>+int j1939_netdev_start(struct net *net, struct net_device *);
>+void j1939_netdev_stop(struct net *net, struct net_device *);
>
> void __j1939_priv_release(struct kref *kref);
> struct j1939_priv *j1939_priv_get(struct net_device *dev);
>-struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex);
>+struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int
>ifindex);
>
>
> static inline void j1939_priv_set(struct net_device *dev, struct
>j1939_priv *priv)
>diff --git a/j1939/main.c b/j1939/main.c
>index 736e9ac8090c..0e55569fb1e6 100644
>--- a/j1939/main.c
>+++ b/j1939/main.c
>@@ -196,7 +196,7 @@ static void j1939_priv_ac_task(unsigned long val)
>
> static DEFINE_SPINLOCK(j1939_netdev_lock);
>
>-int j1939_netdev_start(struct net_device *netdev)
>+int j1939_netdev_start(struct net *net, struct net_device *netdev)
> {
> struct j1939_priv *priv;
> int ret;
>@@ -221,7 +221,7 @@ int j1939_netdev_start(struct net_device *netdev)
> dev_hold(netdev);
>
> /* add CAN handler */
>- ret = can_rx_register(&init_net, netdev, J1939_CAN_ID,
>J1939_CAN_MASK,
>+ ret = can_rx_register(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
> j1939_can_recv, priv, "j1939", NULL);
> if (ret < 0)
> goto out_dev_put;
>@@ -229,7 +229,8 @@ int j1939_netdev_start(struct net_device *netdev)
> spin_lock(&j1939_netdev_lock);
> if (j1939_priv_get(netdev)) {
> /* Someone was faster than us, use their priv and roll
>- * back our's. */
>+ * back our's.
>+ */
> spin_unlock(&j1939_netdev_lock);
> goto out_rx_unregister;
> }
>@@ -239,7 +240,7 @@ int j1939_netdev_start(struct net_device *netdev)
> return 0;
>
> out_rx_unregister:
>- can_rx_unregister(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>+ can_rx_unregister(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
> j1939_can_recv, priv);
> out_dev_put:
> dev_put(netdev);
>@@ -248,12 +249,14 @@ int j1939_netdev_start(struct net_device *netdev)
> return ret;
> }
>
>-void j1939_netdev_stop(struct net_device *netdev)
>+void j1939_netdev_stop(struct net *net, struct net_device *netdev)
> {
> struct j1939_priv *priv;
>
> spin_lock(&j1939_netdev_lock);
> priv = __j1939_priv_get(netdev);
>+ can_rx_unregister(net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
>+ j1939_can_recv, priv);
> j1939_priv_put(priv);
> spin_unlock(&j1939_netdev_lock);
> }
>@@ -264,9 +267,6 @@ void __j1939_priv_release(struct kref *kref)
> struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
> struct j1939_ecu *ecu;
>
>- can_rx_unregister(&init_net, priv->netdev, J1939_CAN_ID,
>J1939_CAN_MASK,
>- j1939_can_recv, priv);
>-
> tasklet_disable_nosync(&priv->ac_task);
>
> /* remove pending transport protocol sessions */
>@@ -302,12 +302,12 @@ struct j1939_priv *j1939_priv_get(struct
>net_device *dev)
> return priv;
> }
>
>-struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex)
>+struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int
>ifindex)
> {
> struct j1939_priv *priv;
> struct net_device *netdev;
>
>- netdev = dev_get_by_index(&init_net, ifindex);
>+ netdev = dev_get_by_index(net, ifindex);
> if (!netdev)
> return NULL;
>
>@@ -321,8 +321,9 @@ static int j1939_netdev_notify(struct
>notifier_block *nb,
> unsigned long msg, void *data)
> {
> struct net_device *netdev = netdev_notifier_info_to_dev(data);
>+ struct net *net = dev_net(netdev);
>
>- if (!net_eq(dev_net(netdev), &init_net))
>+ if (!net_eq(dev_net(netdev), net))
> return NOTIFY_DONE;
>
> if (netdev->type != ARPHRD_CAN)
>diff --git a/j1939/socket.c b/j1939/socket.c
>index ae5357510623..fe70411cdf88 100644
>--- a/j1939/socket.c
>+++ b/j1939/socket.c
>@@ -200,6 +200,7 @@ static int j1939sk_init(struct sock *sk)
>static int j1939sk_bind(struct socket *sock, struct sockaddr *uaddr,
>int len)
> {
> struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
>+ struct sock *sk = sock->sk;
> struct j1939_sock *jsk = j1939_sk(sock->sk);
> struct net_device *netdev;
> struct j1939_priv *priv;
>@@ -219,7 +220,7 @@ static int j1939sk_bind(struct socket *sock,
>struct sockaddr *uaddr, int len)
>
> lock_sock(sock->sk);
>
>- netdev = dev_get_by_index(&init_net, addr->can_ifindex);
>+ netdev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
> if (!netdev) {
> ret = -ENODEV;
> goto out_release_sock;
>@@ -245,8 +246,8 @@ static int j1939sk_bind(struct socket *sock,
>struct sockaddr *uaddr, int len)
> goto out_dev_put;
> }
>
>- ret = j1939_netdev_start(netdev);
>- if (ret < 0)
>+ ret = j1939_netdev_start(sock_net(sk), netdev);
>+ if (ret < 0)
> goto out_dev_put;
>
> jsk->sk.sk_bound_dev_if = addr->can_ifindex;
>@@ -384,14 +385,14 @@ static int j1939sk_release(struct socket *sock)
> list_del_init(&jsk->list);
> spin_unlock_bh(&j1939_socks_lock);
>
>- netdev = dev_get_by_index(&init_net, jsk->sk.sk_bound_dev_if);
>+ netdev = dev_get_by_index(sock_net(sk),
>jsk->sk.sk_bound_dev_if);
> if (netdev) {
> priv = j1939_priv_get(netdev);
> j1939_addr_local_put(priv, jsk->addr.sa);
> j1939_name_local_put(priv, jsk->addr.src_name);
> j1939_priv_put(priv);
>
>- j1939_netdev_stop(netdev);
>+ j1939_netdev_stop(sock_net(sk), netdev);
> dev_put(netdev);
> }
> }
>@@ -457,9 +458,11 @@ static int j1939sk_setsockopt(struct socket
>*sock, int level, int optname,
> kfree(ofilters);
> return 0;
> case SO_J1939_PROMISC:
>- return j1939sk_setsockopt_flag(jsk, optval, optlen,
>J1939_SOCK_PROMISC);
>+ return j1939sk_setsockopt_flag(jsk, optval, optlen,
>+ J1939_SOCK_PROMISC);
> case SO_J1939_RECV_OWN:
>- return j1939sk_setsockopt_flag(jsk, optval, optlen,
>J1939_SOCK_RECV_OWN);
>+ return j1939sk_setsockopt_flag(jsk, optval, optlen,
>+ J1939_SOCK_RECV_OWN);
> case SO_J1939_SEND_PRIO:
> if (optlen != sizeof(tmp))
> return -EINVAL;
>@@ -618,7 +621,7 @@ static int j1939sk_sendmsg(struct socket *sock,
>struct msghdr *msg, size_t size)
> return -EBADFD;
> }
>
>- dev = dev_get_by_index(&init_net, ifindex);
>+ dev = dev_get_by_index(sock_net(sk), ifindex);
> if (!dev)
> return -ENXIO;
>
>@@ -714,7 +717,7 @@ void j1939sk_netdev_event(struct net_device
>*netdev, int error_code)
> j1939_name_local_put(priv, jsk->addr.src_name);
> j1939_priv_put(priv);
>
>- j1939_netdev_stop(netdev);
>+ j1939_netdev_stop(sock_net(&jsk->sk), netdev);
> }
> /* do not remove filters here */
> }
>diff --git a/j1939/transport.c b/j1939/transport.c
>index fbebcc20bccf..67722773c0b0 100644
>--- a/j1939/transport.c
>+++ b/j1939/transport.c
>@@ -107,8 +107,8 @@ struct session {
> /* forward declarations */
> static struct session *j1939_session_new(struct sk_buff *skb);
> static struct session *j1939_session_fresh_new(int size,
>- struct sk_buff *rel_skb,
>- pgn_t pgn);
>+ struct sk_buff *rel_skb,
>+ pgn_t pgn);
> static void j1939tp_del_work(struct work_struct *work);
>
> /* local variables */
>@@ -1152,7 +1152,7 @@ int j1939_send_transport(struct sk_buff *skb)
> return ret;
>
> /* fix dst_flags, it may be used there soon */
>- priv = j1939_priv_get_by_ifindex(can_skb_prv(skb)->ifindex);
>+ priv = j1939_priv_get_by_ifindex(dev_net(skb->dev),
>can_skb_prv(skb)->ifindex);
> if (!priv)
> return -EINVAL;
> if (j1939_address_is_unicast(cb->addr.da) &&
>@@ -1273,8 +1273,8 @@ int j1939_recv_transport(struct sk_buff *skb)
> }
>
> static struct session *j1939_session_fresh_new(int size,
>- struct sk_buff *rel_skb,
>- pgn_t pgn)
>+ struct sk_buff *rel_skb,
>+ pgn_t pgn)
> {
> struct sk_buff *skb;
> struct j1939_sk_buff_cb *cb;
>@@ -1400,6 +1400,7 @@ static struct ctl_table_header *sysctl_hdr;
> /* module init */
> int __init j1939tp_module_init(void)
> {
>+ //TODO: not sure how to make this network namespace-aware??
> sysctl_hdr = register_net_sysctl(&init_net, "net/can-j1939",
> canj1939_sysctl_table);
> if (!sysctl_hdr)
>
>
>---
>--
>To unsubscribe from this list: send the line "unsubscribe linux-can" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
Sent from a small mobile device
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-05 20:31 j1939: Attempt to make j1939 network namespace-aware Elenita Hinds
2018-02-05 20:44 ` Kurt Van Dijck
@ 2018-02-06 8:52 ` Robert Schwebel
2018-02-06 20:37 ` Kurt Van Dijck
2 siblings, 0 replies; 12+ messages in thread
From: Robert Schwebel @ 2018-02-06 8:52 UTC (permalink / raw)
To: Elenita Hinds; +Cc: linux-can, dev.kurt
On Mon, Feb 05, 2018 at 02:31:32PM -0600, Elenita Hinds wrote:
> Below is a first attempt to make the j1939 kernel support be network
> namespace-aware (needed for my project). Please review, especially
> Kurt and Marc, as there maybe better ways of doing this. I'll
> re-submit a more official patch after getting feedback/comments.
>
> Please note the TODO in j1939/transport.c, j1939tp_module_init().
Oleksij is also working on namespace support recently; unfortunately, he
is ill this week, so it might take some time until he'll answer your
mail. Please synchronize with him.
rsc
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-05 20:44 ` Kurt Van Dijck
@ 2018-02-06 16:15 ` Elenita Hinds
2018-02-06 18:30 ` Oliver Hartkopp
0 siblings, 1 reply; 12+ messages in thread
From: Elenita Hinds @ 2018-02-06 16:15 UTC (permalink / raw)
To: Kurt Van Dijck; +Cc: linux-can
Hi Kurt,
What we've done is create a separate namespace for the CAN interface
so that only certain processes assigned in that namespace have access
to this resource. My understanding is this network namespace provides
a brand-new copy of the network stack (will have its own routes,
firewall rules, network devices) for the processes belonging in that
namespace. init_net points to the default namespace created by the
init process and will not have access or know about the network
interfaces. So as it is written now, socket APIs such as 'bind()'
will return an error (i.e., no such device) as it does not know of the
can0 or can1 interface devices.
The changes attempted here is similar to the raw CAN implementation
and other network stacks, wherein the 'struct net' data structure
needed by the various kernel calls is retrieved from the socket data
structure 'struct sock', or passed as an argument as needed. This
'struct net' variable represents the network device instance the
calling process is using. It is used to call the kernel functions
instead of passing 'init_net'.
Hope this helps.
--elenita
On Mon, Feb 5, 2018 at 2:44 PM, Kurt Van Dijck
<dev.kurt@vandijck-laurijssen.be> wrote:
> Hey,
> I'm not so used to containers. Can you explain a bit what I should expect from network namespaces wrt. J1939? What is unique, what will be duplicated, ...
> That will make the review easier, as I will understand better the purpose than I do now.
> Kurt
>
> On 5 February 2018 21:31:32 CET, Elenita Hinds <ecathinds@gmail.com> wrote:
>>Hi all,
>>
>>Below is a first attempt to make the j1939 kernel support be network
>>namespace-aware (needed for my project). Please review, especially
>>Kurt and Marc, as there maybe better ways of doing this. I'll
>>re-submit a more official patch after getting feedback/comments.
>>
>>Please note the TODO in j1939/transport.c, j1939tp_module_init().
>>
>>Regards,
>>Elenita Hinds
>>
>>---
>>diff --git a/j1939/address-claim.c b/j1939/address-claim.c
>>index ab538e1c4906..784e91762c6e 100644
>>--- a/j1939/address-claim.c
>>+++ b/j1939/address-claim.c
>>@@ -89,8 +89,9 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>> /* return both when failure & when successful */
>> if (ret < 0)
>> return ret;
>>- ecu = j1939_ecu_find_by_name(skcb->addr.src_name,
>>- skb->dev->ifindex);
>>+ ecu = j1939_ecu_find_by_name(dev_net(skb->dev),
>>+ skcb->addr.src_name,
>>+ skb->dev->ifindex);
>> if (!ecu)
>> return -ENODEV;
>>
>>@@ -100,7 +101,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>> put_j1939_ecu(ecu);
>> } else if (skcb->addr.src_name) {
>> /* assign source address */
>>- sa = j1939_name_to_sa(skcb->addr.src_name, skb->dev->ifindex);
>>+ sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.src_name,
>>+ skb->dev->ifindex);
>> if (!j1939_address_is_unicast(sa) &&
>> !ac_msg_is_request_for_ac(skb)) {
>> pr_notice("tx drop: invalid sa for name 0x%016llx\n",
>>@@ -112,7 +114,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>>
>> /* assign destination address */
>> if (skcb->addr.dst_name) {
>>- sa = j1939_name_to_sa(skcb->addr.dst_name, skb->dev->ifindex);
>>+ sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.dst_name,
>>+ skb->dev->ifindex);
>> if (!j1939_address_is_unicast(sa)) {
>> pr_notice("tx drop: invalid da for name 0x%016llx\n",
>> skcb->addr.dst_name);
>>@@ -123,7 +126,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>> return 0;
>> }
>>
>>-static void j1939_process_address_claim(struct sk_buff *skb, struct
>>j1939_priv *priv)
>>+static void j1939_process_address_claim(struct sk_buff *skb,
>>+ struct j1939_priv *priv)
>> {
>> struct j1939_sk_buff_cb *skcb = j1939_get_cb(skb);
>> struct j1939_ecu *ecu, *prev;
>>diff --git a/j1939/bus.c b/j1939/bus.c
>>index 9ba6da216c1b..1af93eb4d939 100644
>>--- a/j1939/bus.c
>>+++ b/j1939/bus.c
>>@@ -110,7 +110,7 @@ struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa,
>>struct j1939_priv *priv)
>> return ecu;
>> }
>>
>>-u8 j1939_name_to_sa(name_t name, int ifindex)
>>+u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex)
>> {
>> struct j1939_ecu *ecu;
>> struct j1939_priv *priv;
>>@@ -118,7 +118,7 @@ u8 j1939_name_to_sa(name_t name, int ifindex)
>>
>> if (!name)
>> return J1939_NO_ADDR;
>>- priv = j1939_priv_get_by_ifindex(ifindex);
>>+ priv = j1939_priv_get_by_ifindex(net, ifindex);
>> if (!priv)
>> return J1939_NO_ADDR;
>>
>>@@ -157,7 +157,7 @@ static struct j1939_ecu
>>*_j1939_ecu_find_by_name(name_t name,
>> }
>>
>> /* ecu lookup by name */
>>-struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex)
>>+struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
>>name, int ifindex)
>> {
>> struct j1939_ecu *ecu;
>> struct j1939_priv *priv;
>>@@ -166,7 +166,7 @@ struct j1939_ecu *j1939_ecu_find_by_name(name_t
>>name, int ifindex)
>> return NULL;
>> if (!ifindex)
>> return NULL;
>>- priv = j1939_priv_get_by_ifindex(ifindex);
>>+ priv = j1939_priv_get_by_ifindex(net, ifindex);
>> if (!priv)
>> return NULL;
>> ecu = _j1939_ecu_find_by_name(name, priv);
>>diff --git a/j1939/j1939-priv.h b/j1939/j1939-priv.h
>>index 2e54092e946a..95288dbc5a90 100644
>>--- a/j1939/j1939-priv.h
>>+++ b/j1939/j1939-priv.h
>>@@ -174,9 +174,9 @@ static inline void j1939_ecu_remove_sa(struct
>>j1939_ecu *ecu)
>> write_unlock_bh(&ecu->priv->lock);
>> }
>>
>>-u8 j1939_name_to_sa(name_t name, int ifindex);
>>+u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex);
>>struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa, struct j1939_priv
>>*priv);
>>-struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex);
>>+struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
>>name, int ifindex);
>> /* find_by_name, with kref & read_lock taken */
>>struct j1939_ecu *j1939_ecu_find_priv_default_tx(int ifindex, name_t
>>*pname,
>> u8 *paddr);
>>@@ -245,12 +245,12 @@ struct j1939_ecu *_j1939_ecu_get_register(struct
>>j1939_priv *priv,
>> /* unregister must be called with lock held */
>> void _j1939_ecu_unregister(struct j1939_ecu *);
>>
>>-int j1939_netdev_start(struct net_device *);
>>-void j1939_netdev_stop(struct net_device *);
>>+int j1939_netdev_start(struct net *net, struct net_device *);
>>+void j1939_netdev_stop(struct net *net, struct net_device *);
>>
>> void __j1939_priv_release(struct kref *kref);
>> struct j1939_priv *j1939_priv_get(struct net_device *dev);
>>-struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex);
>>+struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int
>>ifindex);
>>
>>
>> static inline void j1939_priv_set(struct net_device *dev, struct
>>j1939_priv *priv)
>>diff --git a/j1939/main.c b/j1939/main.c
>>index 736e9ac8090c..0e55569fb1e6 100644
>>--- a/j1939/main.c
>>+++ b/j1939/main.c
>>@@ -196,7 +196,7 @@ static void j1939_priv_ac_task(unsigned long val)
>>
>> static DEFINE_SPINLOCK(j1939_netdev_lock);
>>
>>-int j1939_netdev_start(struct net_device *netdev)
>>+int j1939_netdev_start(struct net *net, struct net_device *netdev)
>> {
>> struct j1939_priv *priv;
>> int ret;
>>@@ -221,7 +221,7 @@ int j1939_netdev_start(struct net_device *netdev)
>> dev_hold(netdev);
>>
>> /* add CAN handler */
>>- ret = can_rx_register(&init_net, netdev, J1939_CAN_ID,
>>J1939_CAN_MASK,
>>+ ret = can_rx_register(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>> j1939_can_recv, priv, "j1939", NULL);
>> if (ret < 0)
>> goto out_dev_put;
>>@@ -229,7 +229,8 @@ int j1939_netdev_start(struct net_device *netdev)
>> spin_lock(&j1939_netdev_lock);
>> if (j1939_priv_get(netdev)) {
>> /* Someone was faster than us, use their priv and roll
>>- * back our's. */
>>+ * back our's.
>>+ */
>> spin_unlock(&j1939_netdev_lock);
>> goto out_rx_unregister;
>> }
>>@@ -239,7 +240,7 @@ int j1939_netdev_start(struct net_device *netdev)
>> return 0;
>>
>> out_rx_unregister:
>>- can_rx_unregister(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>>+ can_rx_unregister(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>> j1939_can_recv, priv);
>> out_dev_put:
>> dev_put(netdev);
>>@@ -248,12 +249,14 @@ int j1939_netdev_start(struct net_device *netdev)
>> return ret;
>> }
>>
>>-void j1939_netdev_stop(struct net_device *netdev)
>>+void j1939_netdev_stop(struct net *net, struct net_device *netdev)
>> {
>> struct j1939_priv *priv;
>>
>> spin_lock(&j1939_netdev_lock);
>> priv = __j1939_priv_get(netdev);
>>+ can_rx_unregister(net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
>>+ j1939_can_recv, priv);
>> j1939_priv_put(priv);
>> spin_unlock(&j1939_netdev_lock);
>> }
>>@@ -264,9 +267,6 @@ void __j1939_priv_release(struct kref *kref)
>> struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
>> struct j1939_ecu *ecu;
>>
>>- can_rx_unregister(&init_net, priv->netdev, J1939_CAN_ID,
>>J1939_CAN_MASK,
>>- j1939_can_recv, priv);
>>-
>> tasklet_disable_nosync(&priv->ac_task);
>>
>> /* remove pending transport protocol sessions */
>>@@ -302,12 +302,12 @@ struct j1939_priv *j1939_priv_get(struct
>>net_device *dev)
>> return priv;
>> }
>>
>>-struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex)
>>+struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int
>>ifindex)
>> {
>> struct j1939_priv *priv;
>> struct net_device *netdev;
>>
>>- netdev = dev_get_by_index(&init_net, ifindex);
>>+ netdev = dev_get_by_index(net, ifindex);
>> if (!netdev)
>> return NULL;
>>
>>@@ -321,8 +321,9 @@ static int j1939_netdev_notify(struct
>>notifier_block *nb,
>> unsigned long msg, void *data)
>> {
>> struct net_device *netdev = netdev_notifier_info_to_dev(data);
>>+ struct net *net = dev_net(netdev);
>>
>>- if (!net_eq(dev_net(netdev), &init_net))
>>+ if (!net_eq(dev_net(netdev), net))
>> return NOTIFY_DONE;
>>
>> if (netdev->type != ARPHRD_CAN)
>>diff --git a/j1939/socket.c b/j1939/socket.c
>>index ae5357510623..fe70411cdf88 100644
>>--- a/j1939/socket.c
>>+++ b/j1939/socket.c
>>@@ -200,6 +200,7 @@ static int j1939sk_init(struct sock *sk)
>>static int j1939sk_bind(struct socket *sock, struct sockaddr *uaddr,
>>int len)
>> {
>> struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
>>+ struct sock *sk = sock->sk;
>> struct j1939_sock *jsk = j1939_sk(sock->sk);
>> struct net_device *netdev;
>> struct j1939_priv *priv;
>>@@ -219,7 +220,7 @@ static int j1939sk_bind(struct socket *sock,
>>struct sockaddr *uaddr, int len)
>>
>> lock_sock(sock->sk);
>>
>>- netdev = dev_get_by_index(&init_net, addr->can_ifindex);
>>+ netdev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
>> if (!netdev) {
>> ret = -ENODEV;
>> goto out_release_sock;
>>@@ -245,8 +246,8 @@ static int j1939sk_bind(struct socket *sock,
>>struct sockaddr *uaddr, int len)
>> goto out_dev_put;
>> }
>>
>>- ret = j1939_netdev_start(netdev);
>>- if (ret < 0)
>>+ ret = j1939_netdev_start(sock_net(sk), netdev);
>>+ if (ret < 0)
>> goto out_dev_put;
>>
>> jsk->sk.sk_bound_dev_if = addr->can_ifindex;
>>@@ -384,14 +385,14 @@ static int j1939sk_release(struct socket *sock)
>> list_del_init(&jsk->list);
>> spin_unlock_bh(&j1939_socks_lock);
>>
>>- netdev = dev_get_by_index(&init_net, jsk->sk.sk_bound_dev_if);
>>+ netdev = dev_get_by_index(sock_net(sk),
>>jsk->sk.sk_bound_dev_if);
>> if (netdev) {
>> priv = j1939_priv_get(netdev);
>> j1939_addr_local_put(priv, jsk->addr.sa);
>> j1939_name_local_put(priv, jsk->addr.src_name);
>> j1939_priv_put(priv);
>>
>>- j1939_netdev_stop(netdev);
>>+ j1939_netdev_stop(sock_net(sk), netdev);
>> dev_put(netdev);
>> }
>> }
>>@@ -457,9 +458,11 @@ static int j1939sk_setsockopt(struct socket
>>*sock, int level, int optname,
>> kfree(ofilters);
>> return 0;
>> case SO_J1939_PROMISC:
>>- return j1939sk_setsockopt_flag(jsk, optval, optlen,
>>J1939_SOCK_PROMISC);
>>+ return j1939sk_setsockopt_flag(jsk, optval, optlen,
>>+ J1939_SOCK_PROMISC);
>> case SO_J1939_RECV_OWN:
>>- return j1939sk_setsockopt_flag(jsk, optval, optlen,
>>J1939_SOCK_RECV_OWN);
>>+ return j1939sk_setsockopt_flag(jsk, optval, optlen,
>>+ J1939_SOCK_RECV_OWN);
>> case SO_J1939_SEND_PRIO:
>> if (optlen != sizeof(tmp))
>> return -EINVAL;
>>@@ -618,7 +621,7 @@ static int j1939sk_sendmsg(struct socket *sock,
>>struct msghdr *msg, size_t size)
>> return -EBADFD;
>> }
>>
>>- dev = dev_get_by_index(&init_net, ifindex);
>>+ dev = dev_get_by_index(sock_net(sk), ifindex);
>> if (!dev)
>> return -ENXIO;
>>
>>@@ -714,7 +717,7 @@ void j1939sk_netdev_event(struct net_device
>>*netdev, int error_code)
>> j1939_name_local_put(priv, jsk->addr.src_name);
>> j1939_priv_put(priv);
>>
>>- j1939_netdev_stop(netdev);
>>+ j1939_netdev_stop(sock_net(&jsk->sk), netdev);
>> }
>> /* do not remove filters here */
>> }
>>diff --git a/j1939/transport.c b/j1939/transport.c
>>index fbebcc20bccf..67722773c0b0 100644
>>--- a/j1939/transport.c
>>+++ b/j1939/transport.c
>>@@ -107,8 +107,8 @@ struct session {
>> /* forward declarations */
>> static struct session *j1939_session_new(struct sk_buff *skb);
>> static struct session *j1939_session_fresh_new(int size,
>>- struct sk_buff *rel_skb,
>>- pgn_t pgn);
>>+ struct sk_buff *rel_skb,
>>+ pgn_t pgn);
>> static void j1939tp_del_work(struct work_struct *work);
>>
>> /* local variables */
>>@@ -1152,7 +1152,7 @@ int j1939_send_transport(struct sk_buff *skb)
>> return ret;
>>
>> /* fix dst_flags, it may be used there soon */
>>- priv = j1939_priv_get_by_ifindex(can_skb_prv(skb)->ifindex);
>>+ priv = j1939_priv_get_by_ifindex(dev_net(skb->dev),
>>can_skb_prv(skb)->ifindex);
>> if (!priv)
>> return -EINVAL;
>> if (j1939_address_is_unicast(cb->addr.da) &&
>>@@ -1273,8 +1273,8 @@ int j1939_recv_transport(struct sk_buff *skb)
>> }
>>
>> static struct session *j1939_session_fresh_new(int size,
>>- struct sk_buff *rel_skb,
>>- pgn_t pgn)
>>+ struct sk_buff *rel_skb,
>>+ pgn_t pgn)
>> {
>> struct sk_buff *skb;
>> struct j1939_sk_buff_cb *cb;
>>@@ -1400,6 +1400,7 @@ static struct ctl_table_header *sysctl_hdr;
>> /* module init */
>> int __init j1939tp_module_init(void)
>> {
>>+ //TODO: not sure how to make this network namespace-aware??
>> sysctl_hdr = register_net_sysctl(&init_net, "net/can-j1939",
>> canj1939_sysctl_table);
>> if (!sysctl_hdr)
>>
>>
>>---
>>--
>>To unsubscribe from this list: send the line "unsubscribe linux-can" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Sent from a small mobile device
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-06 16:15 ` Elenita Hinds
@ 2018-02-06 18:30 ` Oliver Hartkopp
2018-02-06 20:16 ` Kurt Van Dijck
0 siblings, 1 reply; 12+ messages in thread
From: Oliver Hartkopp @ 2018-02-06 18:30 UTC (permalink / raw)
To: Elenita Hinds, Kurt Van Dijck; +Cc: linux-can
Hi Kurt,
Elenita is right. The idea is to have different network namespaces (e.g.
to be used in (Docker-)containers) where each namespace has its own
private view to the network.
You can move real CAN network interfaces from the root namespace into a
different namespace or you can create vcan's inside a network namespace.
https://marc.info/?l=linux-can&m=149046502301622&w=2
To create CAN links between namespaces the vxcan driver can be used
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=a8f820a380a2a06fc4fe1a54159067958f800929
which is similar to veth usage for ethernet/IP traffic.
The CAN namespace support has been introduced in Linux 4.12, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/net/can?id=74b7b490886852582d986a33443c2ffa50970169
The (easy) namespace support for CAN ISO-TP is here:
https://github.com/hartkopp/can-isotp/commit/3a3540c020d3889c392fd355024fd50f73f77afc
Best regards,
Oliver
On 02/06/2018 05:15 PM, Elenita Hinds wrote:
> Hi Kurt,
>
> What we've done is create a separate namespace for the CAN interface
> so that only certain processes assigned in that namespace have access
> to this resource. My understanding is this network namespace provides
> a brand-new copy of the network stack (will have its own routes,
> firewall rules, network devices) for the processes belonging in that
> namespace. init_net points to the default namespace created by the
> init process and will not have access or know about the network
> interfaces. So as it is written now, socket APIs such as 'bind()'
> will return an error (i.e., no such device) as it does not know of the
> can0 or can1 interface devices.
>
> The changes attempted here is similar to the raw CAN implementation
> and other network stacks, wherein the 'struct net' data structure
> needed by the various kernel calls is retrieved from the socket data
> structure 'struct sock', or passed as an argument as needed. This
> 'struct net' variable represents the network device instance the
> calling process is using. It is used to call the kernel functions
> instead of passing 'init_net'.
>
> Hope this helps.
>
> --elenita
>
> On Mon, Feb 5, 2018 at 2:44 PM, Kurt Van Dijck
> <dev.kurt@vandijck-laurijssen.be> wrote:
>> Hey,
>> I'm not so used to containers. Can you explain a bit what I should expect from network namespaces wrt. J1939? What is unique, what will be duplicated, ...
>> That will make the review easier, as I will understand better the purpose than I do now.
>> Kurt
>>
>> On 5 February 2018 21:31:32 CET, Elenita Hinds <ecathinds@gmail.com> wrote:
>>> Hi all,
>>>
>>> Below is a first attempt to make the j1939 kernel support be network
>>> namespace-aware (needed for my project). Please review, especially
>>> Kurt and Marc, as there maybe better ways of doing this. I'll
>>> re-submit a more official patch after getting feedback/comments.
>>>
>>> Please note the TODO in j1939/transport.c, j1939tp_module_init().
>>>
>>> Regards,
>>> Elenita Hinds
>>>
>>> ---
>>> diff --git a/j1939/address-claim.c b/j1939/address-claim.c
>>> index ab538e1c4906..784e91762c6e 100644
>>> --- a/j1939/address-claim.c
>>> +++ b/j1939/address-claim.c
>>> @@ -89,8 +89,9 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>>> /* return both when failure & when successful */
>>> if (ret < 0)
>>> return ret;
>>> - ecu = j1939_ecu_find_by_name(skcb->addr.src_name,
>>> - skb->dev->ifindex);
>>> + ecu = j1939_ecu_find_by_name(dev_net(skb->dev),
>>> + skcb->addr.src_name,
>>> + skb->dev->ifindex);
>>> if (!ecu)
>>> return -ENODEV;
>>>
>>> @@ -100,7 +101,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>>> put_j1939_ecu(ecu);
>>> } else if (skcb->addr.src_name) {
>>> /* assign source address */
>>> - sa = j1939_name_to_sa(skcb->addr.src_name, skb->dev->ifindex);
>>> + sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.src_name,
>>> + skb->dev->ifindex);
>>> if (!j1939_address_is_unicast(sa) &&
>>> !ac_msg_is_request_for_ac(skb)) {
>>> pr_notice("tx drop: invalid sa for name 0x%016llx\n",
>>> @@ -112,7 +114,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>>>
>>> /* assign destination address */
>>> if (skcb->addr.dst_name) {
>>> - sa = j1939_name_to_sa(skcb->addr.dst_name, skb->dev->ifindex);
>>> + sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.dst_name,
>>> + skb->dev->ifindex);
>>> if (!j1939_address_is_unicast(sa)) {
>>> pr_notice("tx drop: invalid da for name 0x%016llx\n",
>>> skcb->addr.dst_name);
>>> @@ -123,7 +126,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>>> return 0;
>>> }
>>>
>>> -static void j1939_process_address_claim(struct sk_buff *skb, struct
>>> j1939_priv *priv)
>>> +static void j1939_process_address_claim(struct sk_buff *skb,
>>> + struct j1939_priv *priv)
>>> {
>>> struct j1939_sk_buff_cb *skcb = j1939_get_cb(skb);
>>> struct j1939_ecu *ecu, *prev;
>>> diff --git a/j1939/bus.c b/j1939/bus.c
>>> index 9ba6da216c1b..1af93eb4d939 100644
>>> --- a/j1939/bus.c
>>> +++ b/j1939/bus.c
>>> @@ -110,7 +110,7 @@ struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa,
>>> struct j1939_priv *priv)
>>> return ecu;
>>> }
>>>
>>> -u8 j1939_name_to_sa(name_t name, int ifindex)
>>> +u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex)
>>> {
>>> struct j1939_ecu *ecu;
>>> struct j1939_priv *priv;
>>> @@ -118,7 +118,7 @@ u8 j1939_name_to_sa(name_t name, int ifindex)
>>>
>>> if (!name)
>>> return J1939_NO_ADDR;
>>> - priv = j1939_priv_get_by_ifindex(ifindex);
>>> + priv = j1939_priv_get_by_ifindex(net, ifindex);
>>> if (!priv)
>>> return J1939_NO_ADDR;
>>>
>>> @@ -157,7 +157,7 @@ static struct j1939_ecu
>>> *_j1939_ecu_find_by_name(name_t name,
>>> }
>>>
>>> /* ecu lookup by name */
>>> -struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex)
>>> +struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
>>> name, int ifindex)
>>> {
>>> struct j1939_ecu *ecu;
>>> struct j1939_priv *priv;
>>> @@ -166,7 +166,7 @@ struct j1939_ecu *j1939_ecu_find_by_name(name_t
>>> name, int ifindex)
>>> return NULL;
>>> if (!ifindex)
>>> return NULL;
>>> - priv = j1939_priv_get_by_ifindex(ifindex);
>>> + priv = j1939_priv_get_by_ifindex(net, ifindex);
>>> if (!priv)
>>> return NULL;
>>> ecu = _j1939_ecu_find_by_name(name, priv);
>>> diff --git a/j1939/j1939-priv.h b/j1939/j1939-priv.h
>>> index 2e54092e946a..95288dbc5a90 100644
>>> --- a/j1939/j1939-priv.h
>>> +++ b/j1939/j1939-priv.h
>>> @@ -174,9 +174,9 @@ static inline void j1939_ecu_remove_sa(struct
>>> j1939_ecu *ecu)
>>> write_unlock_bh(&ecu->priv->lock);
>>> }
>>>
>>> -u8 j1939_name_to_sa(name_t name, int ifindex);
>>> +u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex);
>>> struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa, struct j1939_priv
>>> *priv);
>>> -struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex);
>>> +struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
>>> name, int ifindex);
>>> /* find_by_name, with kref & read_lock taken */
>>> struct j1939_ecu *j1939_ecu_find_priv_default_tx(int ifindex, name_t
>>> *pname,
>>> u8 *paddr);
>>> @@ -245,12 +245,12 @@ struct j1939_ecu *_j1939_ecu_get_register(struct
>>> j1939_priv *priv,
>>> /* unregister must be called with lock held */
>>> void _j1939_ecu_unregister(struct j1939_ecu *);
>>>
>>> -int j1939_netdev_start(struct net_device *);
>>> -void j1939_netdev_stop(struct net_device *);
>>> +int j1939_netdev_start(struct net *net, struct net_device *);
>>> +void j1939_netdev_stop(struct net *net, struct net_device *);
>>>
>>> void __j1939_priv_release(struct kref *kref);
>>> struct j1939_priv *j1939_priv_get(struct net_device *dev);
>>> -struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex);
>>> +struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int
>>> ifindex);
>>>
>>>
>>> static inline void j1939_priv_set(struct net_device *dev, struct
>>> j1939_priv *priv)
>>> diff --git a/j1939/main.c b/j1939/main.c
>>> index 736e9ac8090c..0e55569fb1e6 100644
>>> --- a/j1939/main.c
>>> +++ b/j1939/main.c
>>> @@ -196,7 +196,7 @@ static void j1939_priv_ac_task(unsigned long val)
>>>
>>> static DEFINE_SPINLOCK(j1939_netdev_lock);
>>>
>>> -int j1939_netdev_start(struct net_device *netdev)
>>> +int j1939_netdev_start(struct net *net, struct net_device *netdev)
>>> {
>>> struct j1939_priv *priv;
>>> int ret;
>>> @@ -221,7 +221,7 @@ int j1939_netdev_start(struct net_device *netdev)
>>> dev_hold(netdev);
>>>
>>> /* add CAN handler */
>>> - ret = can_rx_register(&init_net, netdev, J1939_CAN_ID,
>>> J1939_CAN_MASK,
>>> + ret = can_rx_register(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>>> j1939_can_recv, priv, "j1939", NULL);
>>> if (ret < 0)
>>> goto out_dev_put;
>>> @@ -229,7 +229,8 @@ int j1939_netdev_start(struct net_device *netdev)
>>> spin_lock(&j1939_netdev_lock);
>>> if (j1939_priv_get(netdev)) {
>>> /* Someone was faster than us, use their priv and roll
>>> - * back our's. */
>>> + * back our's.
>>> + */
>>> spin_unlock(&j1939_netdev_lock);
>>> goto out_rx_unregister;
>>> }
>>> @@ -239,7 +240,7 @@ int j1939_netdev_start(struct net_device *netdev)
>>> return 0;
>>>
>>> out_rx_unregister:
>>> - can_rx_unregister(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>>> + can_rx_unregister(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>>> j1939_can_recv, priv);
>>> out_dev_put:
>>> dev_put(netdev);
>>> @@ -248,12 +249,14 @@ int j1939_netdev_start(struct net_device *netdev)
>>> return ret;
>>> }
>>>
>>> -void j1939_netdev_stop(struct net_device *netdev)
>>> +void j1939_netdev_stop(struct net *net, struct net_device *netdev)
>>> {
>>> struct j1939_priv *priv;
>>>
>>> spin_lock(&j1939_netdev_lock);
>>> priv = __j1939_priv_get(netdev);
>>> + can_rx_unregister(net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
>>> + j1939_can_recv, priv);
>>> j1939_priv_put(priv);
>>> spin_unlock(&j1939_netdev_lock);
>>> }
>>> @@ -264,9 +267,6 @@ void __j1939_priv_release(struct kref *kref)
>>> struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
>>> struct j1939_ecu *ecu;
>>>
>>> - can_rx_unregister(&init_net, priv->netdev, J1939_CAN_ID,
>>> J1939_CAN_MASK,
>>> - j1939_can_recv, priv);
>>> -
>>> tasklet_disable_nosync(&priv->ac_task);
>>>
>>> /* remove pending transport protocol sessions */
>>> @@ -302,12 +302,12 @@ struct j1939_priv *j1939_priv_get(struct
>>> net_device *dev)
>>> return priv;
>>> }
>>>
>>> -struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex)
>>> +struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int
>>> ifindex)
>>> {
>>> struct j1939_priv *priv;
>>> struct net_device *netdev;
>>>
>>> - netdev = dev_get_by_index(&init_net, ifindex);
>>> + netdev = dev_get_by_index(net, ifindex);
>>> if (!netdev)
>>> return NULL;
>>>
>>> @@ -321,8 +321,9 @@ static int j1939_netdev_notify(struct
>>> notifier_block *nb,
>>> unsigned long msg, void *data)
>>> {
>>> struct net_device *netdev = netdev_notifier_info_to_dev(data);
>>> + struct net *net = dev_net(netdev);
>>>
>>> - if (!net_eq(dev_net(netdev), &init_net))
>>> + if (!net_eq(dev_net(netdev), net))
>>> return NOTIFY_DONE;
>>>
>>> if (netdev->type != ARPHRD_CAN)
>>> diff --git a/j1939/socket.c b/j1939/socket.c
>>> index ae5357510623..fe70411cdf88 100644
>>> --- a/j1939/socket.c
>>> +++ b/j1939/socket.c
>>> @@ -200,6 +200,7 @@ static int j1939sk_init(struct sock *sk)
>>> static int j1939sk_bind(struct socket *sock, struct sockaddr *uaddr,
>>> int len)
>>> {
>>> struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
>>> + struct sock *sk = sock->sk;
>>> struct j1939_sock *jsk = j1939_sk(sock->sk);
>>> struct net_device *netdev;
>>> struct j1939_priv *priv;
>>> @@ -219,7 +220,7 @@ static int j1939sk_bind(struct socket *sock,
>>> struct sockaddr *uaddr, int len)
>>>
>>> lock_sock(sock->sk);
>>>
>>> - netdev = dev_get_by_index(&init_net, addr->can_ifindex);
>>> + netdev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
>>> if (!netdev) {
>>> ret = -ENODEV;
>>> goto out_release_sock;
>>> @@ -245,8 +246,8 @@ static int j1939sk_bind(struct socket *sock,
>>> struct sockaddr *uaddr, int len)
>>> goto out_dev_put;
>>> }
>>>
>>> - ret = j1939_netdev_start(netdev);
>>> - if (ret < 0)
>>> + ret = j1939_netdev_start(sock_net(sk), netdev);
>>> + if (ret < 0)
>>> goto out_dev_put;
>>>
>>> jsk->sk.sk_bound_dev_if = addr->can_ifindex;
>>> @@ -384,14 +385,14 @@ static int j1939sk_release(struct socket *sock)
>>> list_del_init(&jsk->list);
>>> spin_unlock_bh(&j1939_socks_lock);
>>>
>>> - netdev = dev_get_by_index(&init_net, jsk->sk.sk_bound_dev_if);
>>> + netdev = dev_get_by_index(sock_net(sk),
>>> jsk->sk.sk_bound_dev_if);
>>> if (netdev) {
>>> priv = j1939_priv_get(netdev);
>>> j1939_addr_local_put(priv, jsk->addr.sa);
>>> j1939_name_local_put(priv, jsk->addr.src_name);
>>> j1939_priv_put(priv);
>>>
>>> - j1939_netdev_stop(netdev);
>>> + j1939_netdev_stop(sock_net(sk), netdev);
>>> dev_put(netdev);
>>> }
>>> }
>>> @@ -457,9 +458,11 @@ static int j1939sk_setsockopt(struct socket
>>> *sock, int level, int optname,
>>> kfree(ofilters);
>>> return 0;
>>> case SO_J1939_PROMISC:
>>> - return j1939sk_setsockopt_flag(jsk, optval, optlen,
>>> J1939_SOCK_PROMISC);
>>> + return j1939sk_setsockopt_flag(jsk, optval, optlen,
>>> + J1939_SOCK_PROMISC);
>>> case SO_J1939_RECV_OWN:
>>> - return j1939sk_setsockopt_flag(jsk, optval, optlen,
>>> J1939_SOCK_RECV_OWN);
>>> + return j1939sk_setsockopt_flag(jsk, optval, optlen,
>>> + J1939_SOCK_RECV_OWN);
>>> case SO_J1939_SEND_PRIO:
>>> if (optlen != sizeof(tmp))
>>> return -EINVAL;
>>> @@ -618,7 +621,7 @@ static int j1939sk_sendmsg(struct socket *sock,
>>> struct msghdr *msg, size_t size)
>>> return -EBADFD;
>>> }
>>>
>>> - dev = dev_get_by_index(&init_net, ifindex);
>>> + dev = dev_get_by_index(sock_net(sk), ifindex);
>>> if (!dev)
>>> return -ENXIO;
>>>
>>> @@ -714,7 +717,7 @@ void j1939sk_netdev_event(struct net_device
>>> *netdev, int error_code)
>>> j1939_name_local_put(priv, jsk->addr.src_name);
>>> j1939_priv_put(priv);
>>>
>>> - j1939_netdev_stop(netdev);
>>> + j1939_netdev_stop(sock_net(&jsk->sk), netdev);
>>> }
>>> /* do not remove filters here */
>>> }
>>> diff --git a/j1939/transport.c b/j1939/transport.c
>>> index fbebcc20bccf..67722773c0b0 100644
>>> --- a/j1939/transport.c
>>> +++ b/j1939/transport.c
>>> @@ -107,8 +107,8 @@ struct session {
>>> /* forward declarations */
>>> static struct session *j1939_session_new(struct sk_buff *skb);
>>> static struct session *j1939_session_fresh_new(int size,
>>> - struct sk_buff *rel_skb,
>>> - pgn_t pgn);
>>> + struct sk_buff *rel_skb,
>>> + pgn_t pgn);
>>> static void j1939tp_del_work(struct work_struct *work);
>>>
>>> /* local variables */
>>> @@ -1152,7 +1152,7 @@ int j1939_send_transport(struct sk_buff *skb)
>>> return ret;
>>>
>>> /* fix dst_flags, it may be used there soon */
>>> - priv = j1939_priv_get_by_ifindex(can_skb_prv(skb)->ifindex);
>>> + priv = j1939_priv_get_by_ifindex(dev_net(skb->dev),
>>> can_skb_prv(skb)->ifindex);
>>> if (!priv)
>>> return -EINVAL;
>>> if (j1939_address_is_unicast(cb->addr.da) &&
>>> @@ -1273,8 +1273,8 @@ int j1939_recv_transport(struct sk_buff *skb)
>>> }
>>>
>>> static struct session *j1939_session_fresh_new(int size,
>>> - struct sk_buff *rel_skb,
>>> - pgn_t pgn)
>>> + struct sk_buff *rel_skb,
>>> + pgn_t pgn)
>>> {
>>> struct sk_buff *skb;
>>> struct j1939_sk_buff_cb *cb;
>>> @@ -1400,6 +1400,7 @@ static struct ctl_table_header *sysctl_hdr;
>>> /* module init */
>>> int __init j1939tp_module_init(void)
>>> {
>>> + //TODO: not sure how to make this network namespace-aware??
>>> sysctl_hdr = register_net_sysctl(&init_net, "net/can-j1939",
>>> canj1939_sysctl_table);
>>> if (!sysctl_hdr)
>>>
>>>
>>> ---
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> Sent from a small mobile device
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-06 18:30 ` Oliver Hartkopp
@ 2018-02-06 20:16 ` Kurt Van Dijck
2018-02-08 15:05 ` Marc Kleine-Budde
2018-02-08 21:13 ` Oliver Hartkopp
0 siblings, 2 replies; 12+ messages in thread
From: Kurt Van Dijck @ 2018-02-06 20:16 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Elenita Hinds, linux-can
Hi Elenita & Oliver,
On di, 06 feb 2018 19:30:41 +0100, Oliver Hartkopp wrote:
> Hi Kurt,
>
> Elenita is right. The idea is to have different network namespaces (e.g. to
> be used in (Docker-)containers) where each namespace has its own private
> view to the network.
So absolutely nothing is/should be shared between the namespaces
from the userspace point of view?
So the ideal j1939 implementation that shares nothing between
interfaces only needs to forward an extra namespace parameter?
How about ... /proc parameters? or Module parameters?
or is that the main argument against them?
Can different namespaces re-use interface numbers (ifindex)?
Anyway, I can start to take a look.
Kurt
>
> You can move real CAN network interfaces from the root namespace into a
> different namespace or you can create vcan's inside a network namespace.
>
> https://marc.info/?l=linux-can&m=149046502301622&w=2
>
> To create CAN links between namespaces the vxcan driver can be used
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=a8f820a380a2a06fc4fe1a54159067958f800929
>
> which is similar to veth usage for ethernet/IP traffic.
>
> The CAN namespace support has been introduced in Linux 4.12, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/net/can?id=74b7b490886852582d986a33443c2ffa50970169
>
> The (easy) namespace support for CAN ISO-TP is here:
> https://github.com/hartkopp/can-isotp/commit/3a3540c020d3889c392fd355024fd50f73f77afc
>
> Best regards,
> Oliver
>
>
> On 02/06/2018 05:15 PM, Elenita Hinds wrote:
> >Hi Kurt,
> >
> >What we've done is create a separate namespace for the CAN interface
> >so that only certain processes assigned in that namespace have access
> >to this resource. My understanding is this network namespace provides
> >a brand-new copy of the network stack (will have its own routes,
> >firewall rules, network devices) for the processes belonging in that
> >namespace. init_net points to the default namespace created by the
> >init process and will not have access or know about the network
> >interfaces. So as it is written now, socket APIs such as 'bind()'
> >will return an error (i.e., no such device) as it does not know of the
> >can0 or can1 interface devices.
> >
> >The changes attempted here is similar to the raw CAN implementation
> >and other network stacks, wherein the 'struct net' data structure
> >needed by the various kernel calls is retrieved from the socket data
> >structure 'struct sock', or passed as an argument as needed. This
> >'struct net' variable represents the network device instance the
> >calling process is using. It is used to call the kernel functions
> >instead of passing 'init_net'.
> >
> >Hope this helps.
> >
> >--elenita
> >
> >On Mon, Feb 5, 2018 at 2:44 PM, Kurt Van Dijck
> ><dev.kurt@vandijck-laurijssen.be> wrote:
> >>Hey,
> >>I'm not so used to containers. Can you explain a bit what I should expect from network namespaces wrt. J1939? What is unique, what will be duplicated, ...
> >>That will make the review easier, as I will understand better the purpose than I do now.
> >>Kurt
> >>
> >>On 5 February 2018 21:31:32 CET, Elenita Hinds <ecathinds@gmail.com> wrote:
> >>>Hi all,
> >>>
> >>>Below is a first attempt to make the j1939 kernel support be network
> >>>namespace-aware (needed for my project). Please review, especially
> >>>Kurt and Marc, as there maybe better ways of doing this. I'll
> >>>re-submit a more official patch after getting feedback/comments.
> >>>
> >>>Please note the TODO in j1939/transport.c, j1939tp_module_init().
> >>>
> >>>Regards,
> >>>Elenita Hinds
> >>>
> >>>---
> >>>diff --git a/j1939/address-claim.c b/j1939/address-claim.c
> >>>index ab538e1c4906..784e91762c6e 100644
> >>>--- a/j1939/address-claim.c
> >>>+++ b/j1939/address-claim.c
> >>>@@ -89,8 +89,9 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> >>> /* return both when failure & when successful */
> >>> if (ret < 0)
> >>> return ret;
> >>>- ecu = j1939_ecu_find_by_name(skcb->addr.src_name,
> >>>- skb->dev->ifindex);
> >>>+ ecu = j1939_ecu_find_by_name(dev_net(skb->dev),
> >>>+ skcb->addr.src_name,
> >>>+ skb->dev->ifindex);
> >>> if (!ecu)
> >>> return -ENODEV;
> >>>
> >>>@@ -100,7 +101,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> >>> put_j1939_ecu(ecu);
> >>> } else if (skcb->addr.src_name) {
> >>> /* assign source address */
> >>>- sa = j1939_name_to_sa(skcb->addr.src_name, skb->dev->ifindex);
> >>>+ sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.src_name,
> >>>+ skb->dev->ifindex);
> >>> if (!j1939_address_is_unicast(sa) &&
> >>> !ac_msg_is_request_for_ac(skb)) {
> >>> pr_notice("tx drop: invalid sa for name 0x%016llx\n",
> >>>@@ -112,7 +114,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> >>>
> >>> /* assign destination address */
> >>> if (skcb->addr.dst_name) {
> >>>- sa = j1939_name_to_sa(skcb->addr.dst_name, skb->dev->ifindex);
> >>>+ sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.dst_name,
> >>>+ skb->dev->ifindex);
> >>> if (!j1939_address_is_unicast(sa)) {
> >>> pr_notice("tx drop: invalid da for name 0x%016llx\n",
> >>> skcb->addr.dst_name);
> >>>@@ -123,7 +126,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> >>> return 0;
> >>>}
> >>>
> >>>-static void j1939_process_address_claim(struct sk_buff *skb, struct
> >>>j1939_priv *priv)
> >>>+static void j1939_process_address_claim(struct sk_buff *skb,
> >>>+ struct j1939_priv *priv)
> >>>{
> >>> struct j1939_sk_buff_cb *skcb = j1939_get_cb(skb);
> >>> struct j1939_ecu *ecu, *prev;
> >>>diff --git a/j1939/bus.c b/j1939/bus.c
> >>>index 9ba6da216c1b..1af93eb4d939 100644
> >>>--- a/j1939/bus.c
> >>>+++ b/j1939/bus.c
> >>>@@ -110,7 +110,7 @@ struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa,
> >>>struct j1939_priv *priv)
> >>> return ecu;
> >>>}
> >>>
> >>>-u8 j1939_name_to_sa(name_t name, int ifindex)
> >>>+u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex)
> >>>{
> >>> struct j1939_ecu *ecu;
> >>> struct j1939_priv *priv;
> >>>@@ -118,7 +118,7 @@ u8 j1939_name_to_sa(name_t name, int ifindex)
> >>>
> >>> if (!name)
> >>> return J1939_NO_ADDR;
> >>>- priv = j1939_priv_get_by_ifindex(ifindex);
> >>>+ priv = j1939_priv_get_by_ifindex(net, ifindex);
> >>> if (!priv)
> >>> return J1939_NO_ADDR;
> >>>
> >>>@@ -157,7 +157,7 @@ static struct j1939_ecu
> >>>*_j1939_ecu_find_by_name(name_t name,
> >>>}
> >>>
> >>>/* ecu lookup by name */
> >>>-struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex)
> >>>+struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
> >>>name, int ifindex)
> >>>{
> >>> struct j1939_ecu *ecu;
> >>> struct j1939_priv *priv;
> >>>@@ -166,7 +166,7 @@ struct j1939_ecu *j1939_ecu_find_by_name(name_t
> >>>name, int ifindex)
> >>> return NULL;
> >>> if (!ifindex)
> >>> return NULL;
> >>>- priv = j1939_priv_get_by_ifindex(ifindex);
> >>>+ priv = j1939_priv_get_by_ifindex(net, ifindex);
> >>> if (!priv)
> >>> return NULL;
> >>> ecu = _j1939_ecu_find_by_name(name, priv);
> >>>diff --git a/j1939/j1939-priv.h b/j1939/j1939-priv.h
> >>>index 2e54092e946a..95288dbc5a90 100644
> >>>--- a/j1939/j1939-priv.h
> >>>+++ b/j1939/j1939-priv.h
> >>>@@ -174,9 +174,9 @@ static inline void j1939_ecu_remove_sa(struct
> >>>j1939_ecu *ecu)
> >>> write_unlock_bh(&ecu->priv->lock);
> >>>}
> >>>
> >>>-u8 j1939_name_to_sa(name_t name, int ifindex);
> >>>+u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex);
> >>>struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa, struct j1939_priv
> >>>*priv);
> >>>-struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex);
> >>>+struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
> >>>name, int ifindex);
> >>>/* find_by_name, with kref & read_lock taken */
> >>>struct j1939_ecu *j1939_ecu_find_priv_default_tx(int ifindex, name_t
> >>>*pname,
> >>> u8 *paddr);
> >>>@@ -245,12 +245,12 @@ struct j1939_ecu *_j1939_ecu_get_register(struct
> >>>j1939_priv *priv,
> >>>/* unregister must be called with lock held */
> >>>void _j1939_ecu_unregister(struct j1939_ecu *);
> >>>
> >>>-int j1939_netdev_start(struct net_device *);
> >>>-void j1939_netdev_stop(struct net_device *);
> >>>+int j1939_netdev_start(struct net *net, struct net_device *);
> >>>+void j1939_netdev_stop(struct net *net, struct net_device *);
> >>>
> >>>void __j1939_priv_release(struct kref *kref);
> >>>struct j1939_priv *j1939_priv_get(struct net_device *dev);
> >>>-struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex);
> >>>+struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int
> >>>ifindex);
> >>>
> >>>
> >>>static inline void j1939_priv_set(struct net_device *dev, struct
> >>>j1939_priv *priv)
> >>>diff --git a/j1939/main.c b/j1939/main.c
> >>>index 736e9ac8090c..0e55569fb1e6 100644
> >>>--- a/j1939/main.c
> >>>+++ b/j1939/main.c
> >>>@@ -196,7 +196,7 @@ static void j1939_priv_ac_task(unsigned long val)
> >>>
> >>>static DEFINE_SPINLOCK(j1939_netdev_lock);
> >>>
> >>>-int j1939_netdev_start(struct net_device *netdev)
> >>>+int j1939_netdev_start(struct net *net, struct net_device *netdev)
> >>>{
> >>> struct j1939_priv *priv;
> >>> int ret;
> >>>@@ -221,7 +221,7 @@ int j1939_netdev_start(struct net_device *netdev)
> >>> dev_hold(netdev);
> >>>
> >>> /* add CAN handler */
> >>>- ret = can_rx_register(&init_net, netdev, J1939_CAN_ID,
> >>>J1939_CAN_MASK,
> >>>+ ret = can_rx_register(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
> >>> j1939_can_recv, priv, "j1939", NULL);
> >>> if (ret < 0)
> >>> goto out_dev_put;
> >>>@@ -229,7 +229,8 @@ int j1939_netdev_start(struct net_device *netdev)
> >>> spin_lock(&j1939_netdev_lock);
> >>> if (j1939_priv_get(netdev)) {
> >>> /* Someone was faster than us, use their priv and roll
> >>>- * back our's. */
> >>>+ * back our's.
> >>>+ */
> >>> spin_unlock(&j1939_netdev_lock);
> >>> goto out_rx_unregister;
> >>> }
> >>>@@ -239,7 +240,7 @@ int j1939_netdev_start(struct net_device *netdev)
> >>> return 0;
> >>>
> >>> out_rx_unregister:
> >>>- can_rx_unregister(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
> >>>+ can_rx_unregister(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
> >>> j1939_can_recv, priv);
> >>> out_dev_put:
> >>> dev_put(netdev);
> >>>@@ -248,12 +249,14 @@ int j1939_netdev_start(struct net_device *netdev)
> >>> return ret;
> >>>}
> >>>
> >>>-void j1939_netdev_stop(struct net_device *netdev)
> >>>+void j1939_netdev_stop(struct net *net, struct net_device *netdev)
> >>>{
> >>> struct j1939_priv *priv;
> >>>
> >>> spin_lock(&j1939_netdev_lock);
> >>> priv = __j1939_priv_get(netdev);
> >>>+ can_rx_unregister(net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
> >>>+ j1939_can_recv, priv);
> >>> j1939_priv_put(priv);
> >>> spin_unlock(&j1939_netdev_lock);
> >>>}
> >>>@@ -264,9 +267,6 @@ void __j1939_priv_release(struct kref *kref)
> >>>struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
> >>> struct j1939_ecu *ecu;
> >>>
> >>>- can_rx_unregister(&init_net, priv->netdev, J1939_CAN_ID,
> >>>J1939_CAN_MASK,
> >>>- j1939_can_recv, priv);
> >>>-
> >>> tasklet_disable_nosync(&priv->ac_task);
> >>>
> >>> /* remove pending transport protocol sessions */
> >>>@@ -302,12 +302,12 @@ struct j1939_priv *j1939_priv_get(struct
> >>>net_device *dev)
> >>> return priv;
> >>>}
> >>>
> >>>-struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex)
> >>>+struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int
> >>>ifindex)
> >>>{
> >>> struct j1939_priv *priv;
> >>> struct net_device *netdev;
> >>>
> >>>- netdev = dev_get_by_index(&init_net, ifindex);
> >>>+ netdev = dev_get_by_index(net, ifindex);
> >>> if (!netdev)
> >>> return NULL;
> >>>
> >>>@@ -321,8 +321,9 @@ static int j1939_netdev_notify(struct
> >>>notifier_block *nb,
> >>> unsigned long msg, void *data)
> >>>{
> >>> struct net_device *netdev = netdev_notifier_info_to_dev(data);
> >>>+ struct net *net = dev_net(netdev);
> >>>
> >>>- if (!net_eq(dev_net(netdev), &init_net))
> >>>+ if (!net_eq(dev_net(netdev), net))
> >>> return NOTIFY_DONE;
> >>>
> >>> if (netdev->type != ARPHRD_CAN)
> >>>diff --git a/j1939/socket.c b/j1939/socket.c
> >>>index ae5357510623..fe70411cdf88 100644
> >>>--- a/j1939/socket.c
> >>>+++ b/j1939/socket.c
> >>>@@ -200,6 +200,7 @@ static int j1939sk_init(struct sock *sk)
> >>>static int j1939sk_bind(struct socket *sock, struct sockaddr *uaddr,
> >>>int len)
> >>>{
> >>> struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
> >>>+ struct sock *sk = sock->sk;
> >>> struct j1939_sock *jsk = j1939_sk(sock->sk);
> >>> struct net_device *netdev;
> >>> struct j1939_priv *priv;
> >>>@@ -219,7 +220,7 @@ static int j1939sk_bind(struct socket *sock,
> >>>struct sockaddr *uaddr, int len)
> >>>
> >>> lock_sock(sock->sk);
> >>>
> >>>- netdev = dev_get_by_index(&init_net, addr->can_ifindex);
> >>>+ netdev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
> >>> if (!netdev) {
> >>> ret = -ENODEV;
> >>> goto out_release_sock;
> >>>@@ -245,8 +246,8 @@ static int j1939sk_bind(struct socket *sock,
> >>>struct sockaddr *uaddr, int len)
> >>> goto out_dev_put;
> >>> }
> >>>
> >>>- ret = j1939_netdev_start(netdev);
> >>>- if (ret < 0)
> >>>+ ret = j1939_netdev_start(sock_net(sk), netdev);
> >>>+ if (ret < 0)
> >>> goto out_dev_put;
> >>>
> >>> jsk->sk.sk_bound_dev_if = addr->can_ifindex;
> >>>@@ -384,14 +385,14 @@ static int j1939sk_release(struct socket *sock)
> >>> list_del_init(&jsk->list);
> >>> spin_unlock_bh(&j1939_socks_lock);
> >>>
> >>>- netdev = dev_get_by_index(&init_net, jsk->sk.sk_bound_dev_if);
> >>>+ netdev = dev_get_by_index(sock_net(sk),
> >>>jsk->sk.sk_bound_dev_if);
> >>> if (netdev) {
> >>> priv = j1939_priv_get(netdev);
> >>> j1939_addr_local_put(priv, jsk->addr.sa);
> >>> j1939_name_local_put(priv, jsk->addr.src_name);
> >>> j1939_priv_put(priv);
> >>>
> >>>- j1939_netdev_stop(netdev);
> >>>+ j1939_netdev_stop(sock_net(sk), netdev);
> >>> dev_put(netdev);
> >>> }
> >>> }
> >>>@@ -457,9 +458,11 @@ static int j1939sk_setsockopt(struct socket
> >>>*sock, int level, int optname,
> >>> kfree(ofilters);
> >>> return 0;
> >>> case SO_J1939_PROMISC:
> >>>- return j1939sk_setsockopt_flag(jsk, optval, optlen,
> >>>J1939_SOCK_PROMISC);
> >>>+ return j1939sk_setsockopt_flag(jsk, optval, optlen,
> >>>+ J1939_SOCK_PROMISC);
> >>> case SO_J1939_RECV_OWN:
> >>>- return j1939sk_setsockopt_flag(jsk, optval, optlen,
> >>>J1939_SOCK_RECV_OWN);
> >>>+ return j1939sk_setsockopt_flag(jsk, optval, optlen,
> >>>+ J1939_SOCK_RECV_OWN);
> >>> case SO_J1939_SEND_PRIO:
> >>> if (optlen != sizeof(tmp))
> >>> return -EINVAL;
> >>>@@ -618,7 +621,7 @@ static int j1939sk_sendmsg(struct socket *sock,
> >>>struct msghdr *msg, size_t size)
> >>> return -EBADFD;
> >>> }
> >>>
> >>>- dev = dev_get_by_index(&init_net, ifindex);
> >>>+ dev = dev_get_by_index(sock_net(sk), ifindex);
> >>> if (!dev)
> >>> return -ENXIO;
> >>>
> >>>@@ -714,7 +717,7 @@ void j1939sk_netdev_event(struct net_device
> >>>*netdev, int error_code)
> >>> j1939_name_local_put(priv, jsk->addr.src_name);
> >>> j1939_priv_put(priv);
> >>>
> >>>- j1939_netdev_stop(netdev);
> >>>+ j1939_netdev_stop(sock_net(&jsk->sk), netdev);
> >>> }
> >>> /* do not remove filters here */
> >>> }
> >>>diff --git a/j1939/transport.c b/j1939/transport.c
> >>>index fbebcc20bccf..67722773c0b0 100644
> >>>--- a/j1939/transport.c
> >>>+++ b/j1939/transport.c
> >>>@@ -107,8 +107,8 @@ struct session {
> >>>/* forward declarations */
> >>>static struct session *j1939_session_new(struct sk_buff *skb);
> >>>static struct session *j1939_session_fresh_new(int size,
> >>>- struct sk_buff *rel_skb,
> >>>- pgn_t pgn);
> >>>+ struct sk_buff *rel_skb,
> >>>+ pgn_t pgn);
> >>>static void j1939tp_del_work(struct work_struct *work);
> >>>
> >>>/* local variables */
> >>>@@ -1152,7 +1152,7 @@ int j1939_send_transport(struct sk_buff *skb)
> >>> return ret;
> >>>
> >>> /* fix dst_flags, it may be used there soon */
> >>>- priv = j1939_priv_get_by_ifindex(can_skb_prv(skb)->ifindex);
> >>>+ priv = j1939_priv_get_by_ifindex(dev_net(skb->dev),
> >>>can_skb_prv(skb)->ifindex);
> >>> if (!priv)
> >>> return -EINVAL;
> >>> if (j1939_address_is_unicast(cb->addr.da) &&
> >>>@@ -1273,8 +1273,8 @@ int j1939_recv_transport(struct sk_buff *skb)
> >>>}
> >>>
> >>>static struct session *j1939_session_fresh_new(int size,
> >>>- struct sk_buff *rel_skb,
> >>>- pgn_t pgn)
> >>>+ struct sk_buff *rel_skb,
> >>>+ pgn_t pgn)
> >>>{
> >>> struct sk_buff *skb;
> >>> struct j1939_sk_buff_cb *cb;
> >>>@@ -1400,6 +1400,7 @@ static struct ctl_table_header *sysctl_hdr;
> >>>/* module init */
> >>>int __init j1939tp_module_init(void)
> >>>{
> >>>+ //TODO: not sure how to make this network namespace-aware??
> >>> sysctl_hdr = register_net_sysctl(&init_net, "net/can-j1939",
> >>> canj1939_sysctl_table);
> >>> if (!sysctl_hdr)
> >>>
> >>>
> >>>---
> >>>--
> >>>To unsubscribe from this list: send the line "unsubscribe linux-can" in
> >>>the body of a message to majordomo@vger.kernel.org
> >>>More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >>Sent from a small mobile device
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-can" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-05 20:31 j1939: Attempt to make j1939 network namespace-aware Elenita Hinds
2018-02-05 20:44 ` Kurt Van Dijck
2018-02-06 8:52 ` Robert Schwebel
@ 2018-02-06 20:37 ` Kurt Van Dijck
2018-02-07 16:41 ` Elenita Hinds
2 siblings, 1 reply; 12+ messages in thread
From: Kurt Van Dijck @ 2018-02-06 20:37 UTC (permalink / raw)
To: Elenita Hinds; +Cc: linux-can
Hey Elenita,
Great job!
I only have a few minor remarks below.
On ma, 05 feb 2018 14:31:32 -0600, Elenita Hinds wrote:
> Hi all,
>
> Below is a first attempt to make the j1939 kernel support be network
> namespace-aware (needed for my project). Please review, especially
> Kurt and Marc, as there maybe better ways of doing this. I'll
> re-submit a more official patch after getting feedback/comments.
>
> Please note the TODO in j1939/transport.c, j1939tp_module_init().
>
> Regards,
> Elenita Hinds
>
> ---
> diff --git a/j1939/address-claim.c b/j1939/address-claim.c
> index ab538e1c4906..784e91762c6e 100644
> --- a/j1939/address-claim.c
> +++ b/j1939/address-claim.c
> @@ -89,8 +89,9 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> /* return both when failure & when successful */
> if (ret < 0)
> return ret;
> - ecu = j1939_ecu_find_by_name(skcb->addr.src_name,
> - skb->dev->ifindex);
> + ecu = j1939_ecu_find_by_name(dev_net(skb->dev),
> + skcb->addr.src_name,
> + skb->dev->ifindex);
> if (!ecu)
> return -ENODEV;
>
> @@ -100,7 +101,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> put_j1939_ecu(ecu);
> } else if (skcb->addr.src_name) {
> /* assign source address */
> - sa = j1939_name_to_sa(skcb->addr.src_name, skb->dev->ifindex);
> + sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.src_name,
> + skb->dev->ifindex);
> if (!j1939_address_is_unicast(sa) &&
> !ac_msg_is_request_for_ac(skb)) {
> pr_notice("tx drop: invalid sa for name 0x%016llx\n",
> @@ -112,7 +114,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>
> /* assign destination address */
> if (skcb->addr.dst_name) {
> - sa = j1939_name_to_sa(skcb->addr.dst_name, skb->dev->ifindex);
> + sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.dst_name,
> + skb->dev->ifindex);
> if (!j1939_address_is_unicast(sa)) {
> pr_notice("tx drop: invalid da for name 0x%016llx\n",
> skcb->addr.dst_name);
> @@ -123,7 +126,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
> return 0;
> }
>
> -static void j1939_process_address_claim(struct sk_buff *skb, struct
> j1939_priv *priv)
> +static void j1939_process_address_claim(struct sk_buff *skb,
> + struct j1939_priv *priv)
> {
> struct j1939_sk_buff_cb *skcb = j1939_get_cb(skb);
> struct j1939_ecu *ecu, *prev;
> diff --git a/j1939/bus.c b/j1939/bus.c
> index 9ba6da216c1b..1af93eb4d939 100644
> --- a/j1939/bus.c
> +++ b/j1939/bus.c
> @@ -110,7 +110,7 @@ struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa,
> struct j1939_priv *priv)
> return ecu;
> }
>
> -u8 j1939_name_to_sa(name_t name, int ifindex)
> +u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex)
> {
> struct j1939_ecu *ecu;
> struct j1939_priv *priv;
> @@ -118,7 +118,7 @@ u8 j1939_name_to_sa(name_t name, int ifindex)
>
> if (!name)
> return J1939_NO_ADDR;
> - priv = j1939_priv_get_by_ifindex(ifindex);
> + priv = j1939_priv_get_by_ifindex(net, ifindex);
> if (!priv)
> return J1939_NO_ADDR;
>
> @@ -157,7 +157,7 @@ static struct j1939_ecu
> *_j1939_ecu_find_by_name(name_t name,
> }
>
> /* ecu lookup by name */
> -struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex)
> +struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
> name, int ifindex)
> {
> struct j1939_ecu *ecu;
> struct j1939_priv *priv;
> @@ -166,7 +166,7 @@ struct j1939_ecu *j1939_ecu_find_by_name(name_t
> name, int ifindex)
> return NULL;
> if (!ifindex)
> return NULL;
> - priv = j1939_priv_get_by_ifindex(ifindex);
> + priv = j1939_priv_get_by_ifindex(net, ifindex);
> if (!priv)
> return NULL;
> ecu = _j1939_ecu_find_by_name(name, priv);
> diff --git a/j1939/j1939-priv.h b/j1939/j1939-priv.h
> index 2e54092e946a..95288dbc5a90 100644
> --- a/j1939/j1939-priv.h
> +++ b/j1939/j1939-priv.h
> @@ -174,9 +174,9 @@ static inline void j1939_ecu_remove_sa(struct
> j1939_ecu *ecu)
> write_unlock_bh(&ecu->priv->lock);
> }
>
> -u8 j1939_name_to_sa(name_t name, int ifindex);
> +u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex);
> struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa, struct j1939_priv *priv);
> -struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex);
> +struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
> name, int ifindex);
> /* find_by_name, with kref & read_lock taken */
> struct j1939_ecu *j1939_ecu_find_priv_default_tx(int ifindex, name_t *pname,
> u8 *paddr);
> @@ -245,12 +245,12 @@ struct j1939_ecu *_j1939_ecu_get_register(struct
> j1939_priv *priv,
> /* unregister must be called with lock held */
> void _j1939_ecu_unregister(struct j1939_ecu *);
>
> -int j1939_netdev_start(struct net_device *);
> -void j1939_netdev_stop(struct net_device *);
> +int j1939_netdev_start(struct net *net, struct net_device *);
> +void j1939_netdev_stop(struct net *net, struct net_device *);
>
> void __j1939_priv_release(struct kref *kref);
> struct j1939_priv *j1939_priv_get(struct net_device *dev);
> -struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex);
> +struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int ifindex);
>
>
> static inline void j1939_priv_set(struct net_device *dev, struct
> j1939_priv *priv)
> diff --git a/j1939/main.c b/j1939/main.c
> index 736e9ac8090c..0e55569fb1e6 100644
> --- a/j1939/main.c
> +++ b/j1939/main.c
> @@ -196,7 +196,7 @@ static void j1939_priv_ac_task(unsigned long val)
>
> static DEFINE_SPINLOCK(j1939_netdev_lock);
>
> -int j1939_netdev_start(struct net_device *netdev)
> +int j1939_netdev_start(struct net *net, struct net_device *netdev)
> {
> struct j1939_priv *priv;
> int ret;
> @@ -221,7 +221,7 @@ int j1939_netdev_start(struct net_device *netdev)
> dev_hold(netdev);
>
> /* add CAN handler */
> - ret = can_rx_register(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
> + ret = can_rx_register(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
> j1939_can_recv, priv, "j1939", NULL);
> if (ret < 0)
> goto out_dev_put;
> @@ -229,7 +229,8 @@ int j1939_netdev_start(struct net_device *netdev)
> spin_lock(&j1939_netdev_lock);
> if (j1939_priv_get(netdev)) {
> /* Someone was faster than us, use their priv and roll
> - * back our's. */
> + * back our's.
> + */
> spin_unlock(&j1939_netdev_lock);
> goto out_rx_unregister;
> }
> @@ -239,7 +240,7 @@ int j1939_netdev_start(struct net_device *netdev)
> return 0;
>
> out_rx_unregister:
> - can_rx_unregister(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
> + can_rx_unregister(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
> j1939_can_recv, priv);
> out_dev_put:
> dev_put(netdev);
> @@ -248,12 +249,14 @@ int j1939_netdev_start(struct net_device *netdev)
> return ret;
> }
>
> -void j1939_netdev_stop(struct net_device *netdev)
> +void j1939_netdev_stop(struct net *net, struct net_device *netdev)
> {
> struct j1939_priv *priv;
>
> spin_lock(&j1939_netdev_lock);
> priv = __j1939_priv_get(netdev);
> + can_rx_unregister(net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
> + j1939_can_recv, priv);
> j1939_priv_put(priv);
> spin_unlock(&j1939_netdev_lock);
> }
> @@ -264,9 +267,6 @@ void __j1939_priv_release(struct kref *kref)
> struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
> struct j1939_ecu *ecu;
>
> - can_rx_unregister(&init_net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
> - j1939_can_recv, priv);
> -
> tasklet_disable_nosync(&priv->ac_task);
>
> /* remove pending transport protocol sessions */
> @@ -302,12 +302,12 @@ struct j1939_priv *j1939_priv_get(struct net_device *dev)
> return priv;
> }
This looks to me as fixing something and adding namespace support in 1
patch. I think it's better to first add a patch that moves the
can_rx_unregister, seperate from the namespace thing.
>
> -struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex)
> +struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int ifindex)
> {
> struct j1939_priv *priv;
> struct net_device *netdev;
>
> - netdev = dev_get_by_index(&init_net, ifindex);
> + netdev = dev_get_by_index(net, ifindex);
> if (!netdev)
> return NULL;
>
> @@ -321,8 +321,9 @@ static int j1939_netdev_notify(struct notifier_block *nb,
> unsigned long msg, void *data)
> {
> struct net_device *netdev = netdev_notifier_info_to_dev(data);
> + struct net *net = dev_net(netdev);
>
> - if (!net_eq(dev_net(netdev), &init_net))
> + if (!net_eq(dev_net(netdev), net))
> return NOTIFY_DONE;
>
> if (netdev->type != ARPHRD_CAN)
> diff --git a/j1939/socket.c b/j1939/socket.c
> index ae5357510623..fe70411cdf88 100644
> --- a/j1939/socket.c
> +++ b/j1939/socket.c
> @@ -200,6 +200,7 @@ static int j1939sk_init(struct sock *sk)
> static int j1939sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> {
> struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
> + struct sock *sk = sock->sk;
> struct j1939_sock *jsk = j1939_sk(sock->sk);
> struct net_device *netdev;
> struct j1939_priv *priv;
> @@ -219,7 +220,7 @@ static int j1939sk_bind(struct socket *sock,
> struct sockaddr *uaddr, int len)
>
> lock_sock(sock->sk);
>
> - netdev = dev_get_by_index(&init_net, addr->can_ifindex);
> + netdev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
> if (!netdev) {
> ret = -ENODEV;
> goto out_release_sock;
> @@ -245,8 +246,8 @@ static int j1939sk_bind(struct socket *sock,
> struct sockaddr *uaddr, int len)
> goto out_dev_put;
> }
>
> - ret = j1939_netdev_start(netdev);
> - if (ret < 0)
> + ret = j1939_netdev_start(sock_net(sk), netdev);
> + if (ret < 0)
> goto out_dev_put;
>
> jsk->sk.sk_bound_dev_if = addr->can_ifindex;
> @@ -384,14 +385,14 @@ static int j1939sk_release(struct socket *sock)
> list_del_init(&jsk->list);
> spin_unlock_bh(&j1939_socks_lock);
>
> - netdev = dev_get_by_index(&init_net, jsk->sk.sk_bound_dev_if);
> + netdev = dev_get_by_index(sock_net(sk), jsk->sk.sk_bound_dev_if);
> if (netdev) {
> priv = j1939_priv_get(netdev);
> j1939_addr_local_put(priv, jsk->addr.sa);
> j1939_name_local_put(priv, jsk->addr.src_name);
> j1939_priv_put(priv);
>
> - j1939_netdev_stop(netdev);
> + j1939_netdev_stop(sock_net(sk), netdev);
> dev_put(netdev);
> }
> }
> @@ -457,9 +458,11 @@ static int j1939sk_setsockopt(struct socket
> *sock, int level, int optname,
> kfree(ofilters);
> return 0;
> case SO_J1939_PROMISC:
> - return j1939sk_setsockopt_flag(jsk, optval, optlen,
> J1939_SOCK_PROMISC);
> + return j1939sk_setsockopt_flag(jsk, optval, optlen,
> + J1939_SOCK_PROMISC);
> case SO_J1939_RECV_OWN:
> - return j1939sk_setsockopt_flag(jsk, optval, optlen,
> J1939_SOCK_RECV_OWN);
> + return j1939sk_setsockopt_flag(jsk, optval, optlen,
> + J1939_SOCK_RECV_OWN);
> case SO_J1939_SEND_PRIO:
> if (optlen != sizeof(tmp))
> return -EINVAL;
> @@ -618,7 +621,7 @@ static int j1939sk_sendmsg(struct socket *sock,
> struct msghdr *msg, size_t size)
> return -EBADFD;
> }
>
> - dev = dev_get_by_index(&init_net, ifindex);
> + dev = dev_get_by_index(sock_net(sk), ifindex);
> if (!dev)
> return -ENXIO;
>
> @@ -714,7 +717,7 @@ void j1939sk_netdev_event(struct net_device
> *netdev, int error_code)
> j1939_name_local_put(priv, jsk->addr.src_name);
> j1939_priv_put(priv);
>
> - j1939_netdev_stop(netdev);
> + j1939_netdev_stop(sock_net(&jsk->sk), netdev);
> }
> /* do not remove filters here */
> }
> diff --git a/j1939/transport.c b/j1939/transport.c
> index fbebcc20bccf..67722773c0b0 100644
> --- a/j1939/transport.c
> +++ b/j1939/transport.c
> @@ -107,8 +107,8 @@ struct session {
> /* forward declarations */
> static struct session *j1939_session_new(struct sk_buff *skb);
> static struct session *j1939_session_fresh_new(int size,
> - struct sk_buff *rel_skb,
> - pgn_t pgn);
> + struct sk_buff *rel_skb,
> + pgn_t pgn);
> static void j1939tp_del_work(struct work_struct *work);
This is not a namespace thing.
Can you remove it.
Maybe add another seperate patch if the whitespace is really better.
>
> /* local variables */
> @@ -1152,7 +1152,7 @@ int j1939_send_transport(struct sk_buff *skb)
> return ret;
>
> /* fix dst_flags, it may be used there soon */
> - priv = j1939_priv_get_by_ifindex(can_skb_prv(skb)->ifindex);
> + priv = j1939_priv_get_by_ifindex(dev_net(skb->dev),
> can_skb_prv(skb)->ifindex);
> if (!priv)
> return -EINVAL;
> if (j1939_address_is_unicast(cb->addr.da) &&
> @@ -1273,8 +1273,8 @@ int j1939_recv_transport(struct sk_buff *skb)
> }
>
> static struct session *j1939_session_fresh_new(int size,
> - struct sk_buff *rel_skb,
> - pgn_t pgn)
> + struct sk_buff *rel_skb,
> + pgn_t pgn)
Only whitespace ...
> {
> struct sk_buff *skb;
> struct j1939_sk_buff_cb *cb;
> @@ -1400,6 +1400,7 @@ static struct ctl_table_header *sysctl_hdr;
> /* module init */
> int __init j1939tp_module_init(void)
> {
> + //TODO: not sure how to make this network namespace-aware??
The question doesn't belong to the patch, but is valid.
I don't have the answer. I suggest to make the j1939 sysctl
not namespace-aware for a moment, and anyone who does not agree will
know how to fix it?
> sysctl_hdr = register_net_sysctl(&init_net, "net/can-j1939",
> canj1939_sysctl_table);
> if (!sysctl_hdr)
>
>
> ---
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-06 20:37 ` Kurt Van Dijck
@ 2018-02-07 16:41 ` Elenita Hinds
2018-02-08 15:08 ` Marc Kleine-Budde
0 siblings, 1 reply; 12+ messages in thread
From: Elenita Hinds @ 2018-02-07 16:41 UTC (permalink / raw)
To: Elenita Hinds, linux-can
Thanks for the feedback, Kurt. I'll split into separate patches as
suggested for clarity. The additional whitespaces were due to linux
kernel styles imposed upon in my environment (enabled and won't let me
commit unless I align certain lines).
I found missing changes in the j1939 notifier block which I think can
cause the kernel to panic when a CAN bus interface goes down. This is
fixed and will post the changes.
I also figured out how to complete the TODO item and will post these as well.
I'm also trying to coordinate these changes with Oleksij (Rempel) but
he is out for the moment.
To answer your questions above, based on the kernel examples and
implied on the info I've seen, I think almost all need duplicating and
this includes the /proc and module parameters (see detailed
description in http://apprize.info/linux/kernel/15.html). And yes, the
ifindex values can be re-used in different namespaces.
--elenita
On Tue, Feb 6, 2018 at 2:37 PM, Kurt Van Dijck
<dev.kurt@vandijck-laurijssen.be> wrote:
> Hey Elenita,
>
> Great job!
> I only have a few minor remarks below.
>
> On ma, 05 feb 2018 14:31:32 -0600, Elenita Hinds wrote:
>> Hi all,
>>
>> Below is a first attempt to make the j1939 kernel support be network
>> namespace-aware (needed for my project). Please review, especially
>> Kurt and Marc, as there maybe better ways of doing this. I'll
>> re-submit a more official patch after getting feedback/comments.
>>
>> Please note the TODO in j1939/transport.c, j1939tp_module_init().
>>
>> Regards,
>> Elenita Hinds
>>
>> ---
>> diff --git a/j1939/address-claim.c b/j1939/address-claim.c
>> index ab538e1c4906..784e91762c6e 100644
>> --- a/j1939/address-claim.c
>> +++ b/j1939/address-claim.c
>> @@ -89,8 +89,9 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>> /* return both when failure & when successful */
>> if (ret < 0)
>> return ret;
>> - ecu = j1939_ecu_find_by_name(skcb->addr.src_name,
>> - skb->dev->ifindex);
>> + ecu = j1939_ecu_find_by_name(dev_net(skb->dev),
>> + skcb->addr.src_name,
>> + skb->dev->ifindex);
>> if (!ecu)
>> return -ENODEV;
>>
>> @@ -100,7 +101,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>> put_j1939_ecu(ecu);
>> } else if (skcb->addr.src_name) {
>> /* assign source address */
>> - sa = j1939_name_to_sa(skcb->addr.src_name, skb->dev->ifindex);
>> + sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.src_name,
>> + skb->dev->ifindex);
>> if (!j1939_address_is_unicast(sa) &&
>> !ac_msg_is_request_for_ac(skb)) {
>> pr_notice("tx drop: invalid sa for name 0x%016llx\n",
>> @@ -112,7 +114,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>>
>> /* assign destination address */
>> if (skcb->addr.dst_name) {
>> - sa = j1939_name_to_sa(skcb->addr.dst_name, skb->dev->ifindex);
>> + sa = j1939_name_to_sa(dev_net(skb->dev), skcb->addr.dst_name,
>> + skb->dev->ifindex);
>> if (!j1939_address_is_unicast(sa)) {
>> pr_notice("tx drop: invalid da for name 0x%016llx\n",
>> skcb->addr.dst_name);
>> @@ -123,7 +126,8 @@ int j1939_fixup_address_claim(struct sk_buff *skb)
>> return 0;
>> }
>>
>> -static void j1939_process_address_claim(struct sk_buff *skb, struct
>> j1939_priv *priv)
>> +static void j1939_process_address_claim(struct sk_buff *skb,
>> + struct j1939_priv *priv)
>> {
>> struct j1939_sk_buff_cb *skcb = j1939_get_cb(skb);
>> struct j1939_ecu *ecu, *prev;
>> diff --git a/j1939/bus.c b/j1939/bus.c
>> index 9ba6da216c1b..1af93eb4d939 100644
>> --- a/j1939/bus.c
>> +++ b/j1939/bus.c
>> @@ -110,7 +110,7 @@ struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa,
>> struct j1939_priv *priv)
>> return ecu;
>> }
>>
>> -u8 j1939_name_to_sa(name_t name, int ifindex)
>> +u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex)
>> {
>> struct j1939_ecu *ecu;
>> struct j1939_priv *priv;
>> @@ -118,7 +118,7 @@ u8 j1939_name_to_sa(name_t name, int ifindex)
>>
>> if (!name)
>> return J1939_NO_ADDR;
>> - priv = j1939_priv_get_by_ifindex(ifindex);
>> + priv = j1939_priv_get_by_ifindex(net, ifindex);
>> if (!priv)
>> return J1939_NO_ADDR;
>>
>> @@ -157,7 +157,7 @@ static struct j1939_ecu
>> *_j1939_ecu_find_by_name(name_t name,
>> }
>>
>> /* ecu lookup by name */
>> -struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex)
>> +struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
>> name, int ifindex)
>> {
>> struct j1939_ecu *ecu;
>> struct j1939_priv *priv;
>> @@ -166,7 +166,7 @@ struct j1939_ecu *j1939_ecu_find_by_name(name_t
>> name, int ifindex)
>> return NULL;
>> if (!ifindex)
>> return NULL;
>> - priv = j1939_priv_get_by_ifindex(ifindex);
>> + priv = j1939_priv_get_by_ifindex(net, ifindex);
>> if (!priv)
>> return NULL;
>> ecu = _j1939_ecu_find_by_name(name, priv);
>> diff --git a/j1939/j1939-priv.h b/j1939/j1939-priv.h
>> index 2e54092e946a..95288dbc5a90 100644
>> --- a/j1939/j1939-priv.h
>> +++ b/j1939/j1939-priv.h
>> @@ -174,9 +174,9 @@ static inline void j1939_ecu_remove_sa(struct
>> j1939_ecu *ecu)
>> write_unlock_bh(&ecu->priv->lock);
>> }
>>
>> -u8 j1939_name_to_sa(name_t name, int ifindex);
>> +u8 j1939_name_to_sa(struct net *net, name_t name, int ifindex);
>> struct j1939_ecu *_j1939_ecu_find_by_addr(u8 sa, struct j1939_priv *priv);
>> -struct j1939_ecu *j1939_ecu_find_by_name(name_t name, int ifindex);
>> +struct j1939_ecu *j1939_ecu_find_by_name(struct net *net, name_t
>> name, int ifindex);
>> /* find_by_name, with kref & read_lock taken */
>> struct j1939_ecu *j1939_ecu_find_priv_default_tx(int ifindex, name_t *pname,
>> u8 *paddr);
>> @@ -245,12 +245,12 @@ struct j1939_ecu *_j1939_ecu_get_register(struct
>> j1939_priv *priv,
>> /* unregister must be called with lock held */
>> void _j1939_ecu_unregister(struct j1939_ecu *);
>>
>> -int j1939_netdev_start(struct net_device *);
>> -void j1939_netdev_stop(struct net_device *);
>> +int j1939_netdev_start(struct net *net, struct net_device *);
>> +void j1939_netdev_stop(struct net *net, struct net_device *);
>>
>> void __j1939_priv_release(struct kref *kref);
>> struct j1939_priv *j1939_priv_get(struct net_device *dev);
>> -struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex);
>> +struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int ifindex);
>>
>>
>> static inline void j1939_priv_set(struct net_device *dev, struct
>> j1939_priv *priv)
>> diff --git a/j1939/main.c b/j1939/main.c
>> index 736e9ac8090c..0e55569fb1e6 100644
>> --- a/j1939/main.c
>> +++ b/j1939/main.c
>> @@ -196,7 +196,7 @@ static void j1939_priv_ac_task(unsigned long val)
>>
>> static DEFINE_SPINLOCK(j1939_netdev_lock);
>>
>> -int j1939_netdev_start(struct net_device *netdev)
>> +int j1939_netdev_start(struct net *net, struct net_device *netdev)
>> {
>> struct j1939_priv *priv;
>> int ret;
>> @@ -221,7 +221,7 @@ int j1939_netdev_start(struct net_device *netdev)
>> dev_hold(netdev);
>>
>> /* add CAN handler */
>> - ret = can_rx_register(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>> + ret = can_rx_register(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>> j1939_can_recv, priv, "j1939", NULL);
>> if (ret < 0)
>> goto out_dev_put;
>> @@ -229,7 +229,8 @@ int j1939_netdev_start(struct net_device *netdev)
>> spin_lock(&j1939_netdev_lock);
>> if (j1939_priv_get(netdev)) {
>> /* Someone was faster than us, use their priv and roll
>> - * back our's. */
>> + * back our's.
>> + */
>> spin_unlock(&j1939_netdev_lock);
>> goto out_rx_unregister;
>> }
>> @@ -239,7 +240,7 @@ int j1939_netdev_start(struct net_device *netdev)
>> return 0;
>>
>> out_rx_unregister:
>> - can_rx_unregister(&init_net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>> + can_rx_unregister(net, netdev, J1939_CAN_ID, J1939_CAN_MASK,
>> j1939_can_recv, priv);
>> out_dev_put:
>> dev_put(netdev);
>> @@ -248,12 +249,14 @@ int j1939_netdev_start(struct net_device *netdev)
>> return ret;
>> }
>>
>> -void j1939_netdev_stop(struct net_device *netdev)
>> +void j1939_netdev_stop(struct net *net, struct net_device *netdev)
>> {
>> struct j1939_priv *priv;
>>
>> spin_lock(&j1939_netdev_lock);
>> priv = __j1939_priv_get(netdev);
>> + can_rx_unregister(net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
>> + j1939_can_recv, priv);
>> j1939_priv_put(priv);
>> spin_unlock(&j1939_netdev_lock);
>> }
>> @@ -264,9 +267,6 @@ void __j1939_priv_release(struct kref *kref)
>> struct j1939_priv *priv = container_of(kref, struct j1939_priv, kref);
>> struct j1939_ecu *ecu;
>>
>> - can_rx_unregister(&init_net, priv->netdev, J1939_CAN_ID, J1939_CAN_MASK,
>> - j1939_can_recv, priv);
>> -
>> tasklet_disable_nosync(&priv->ac_task);
>>
>> /* remove pending transport protocol sessions */
>> @@ -302,12 +302,12 @@ struct j1939_priv *j1939_priv_get(struct net_device *dev)
>> return priv;
>> }
>
> This looks to me as fixing something and adding namespace support in 1
> patch. I think it's better to first add a patch that moves the
> can_rx_unregister, seperate from the namespace thing.
>
>>
>> -struct j1939_priv *j1939_priv_get_by_ifindex(int ifindex)
>> +struct j1939_priv *j1939_priv_get_by_ifindex(struct net *net, int ifindex)
>> {
>> struct j1939_priv *priv;
>> struct net_device *netdev;
>>
>> - netdev = dev_get_by_index(&init_net, ifindex);
>> + netdev = dev_get_by_index(net, ifindex);
>> if (!netdev)
>> return NULL;
>>
>> @@ -321,8 +321,9 @@ static int j1939_netdev_notify(struct notifier_block *nb,
>> unsigned long msg, void *data)
>> {
>> struct net_device *netdev = netdev_notifier_info_to_dev(data);
>> + struct net *net = dev_net(netdev);
>>
>> - if (!net_eq(dev_net(netdev), &init_net))
>> + if (!net_eq(dev_net(netdev), net))
>> return NOTIFY_DONE;
>>
>> if (netdev->type != ARPHRD_CAN)
>> diff --git a/j1939/socket.c b/j1939/socket.c
>> index ae5357510623..fe70411cdf88 100644
>> --- a/j1939/socket.c
>> +++ b/j1939/socket.c
>> @@ -200,6 +200,7 @@ static int j1939sk_init(struct sock *sk)
>> static int j1939sk_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>> {
>> struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
>> + struct sock *sk = sock->sk;
>> struct j1939_sock *jsk = j1939_sk(sock->sk);
>> struct net_device *netdev;
>> struct j1939_priv *priv;
>> @@ -219,7 +220,7 @@ static int j1939sk_bind(struct socket *sock,
>> struct sockaddr *uaddr, int len)
>>
>> lock_sock(sock->sk);
>>
>> - netdev = dev_get_by_index(&init_net, addr->can_ifindex);
>> + netdev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
>> if (!netdev) {
>> ret = -ENODEV;
>> goto out_release_sock;
>> @@ -245,8 +246,8 @@ static int j1939sk_bind(struct socket *sock,
>> struct sockaddr *uaddr, int len)
>> goto out_dev_put;
>> }
>>
>> - ret = j1939_netdev_start(netdev);
>> - if (ret < 0)
>> + ret = j1939_netdev_start(sock_net(sk), netdev);
>> + if (ret < 0)
>> goto out_dev_put;
>>
>> jsk->sk.sk_bound_dev_if = addr->can_ifindex;
>> @@ -384,14 +385,14 @@ static int j1939sk_release(struct socket *sock)
>> list_del_init(&jsk->list);
>> spin_unlock_bh(&j1939_socks_lock);
>>
>> - netdev = dev_get_by_index(&init_net, jsk->sk.sk_bound_dev_if);
>> + netdev = dev_get_by_index(sock_net(sk), jsk->sk.sk_bound_dev_if);
>> if (netdev) {
>> priv = j1939_priv_get(netdev);
>> j1939_addr_local_put(priv, jsk->addr.sa);
>> j1939_name_local_put(priv, jsk->addr.src_name);
>> j1939_priv_put(priv);
>>
>> - j1939_netdev_stop(netdev);
>> + j1939_netdev_stop(sock_net(sk), netdev);
>> dev_put(netdev);
>> }
>> }
>> @@ -457,9 +458,11 @@ static int j1939sk_setsockopt(struct socket
>> *sock, int level, int optname,
>> kfree(ofilters);
>> return 0;
>> case SO_J1939_PROMISC:
>> - return j1939sk_setsockopt_flag(jsk, optval, optlen,
>> J1939_SOCK_PROMISC);
>> + return j1939sk_setsockopt_flag(jsk, optval, optlen,
>> + J1939_SOCK_PROMISC);
>> case SO_J1939_RECV_OWN:
>> - return j1939sk_setsockopt_flag(jsk, optval, optlen,
>> J1939_SOCK_RECV_OWN);
>> + return j1939sk_setsockopt_flag(jsk, optval, optlen,
>> + J1939_SOCK_RECV_OWN);
>> case SO_J1939_SEND_PRIO:
>> if (optlen != sizeof(tmp))
>> return -EINVAL;
>> @@ -618,7 +621,7 @@ static int j1939sk_sendmsg(struct socket *sock,
>> struct msghdr *msg, size_t size)
>> return -EBADFD;
>> }
>>
>> - dev = dev_get_by_index(&init_net, ifindex);
>> + dev = dev_get_by_index(sock_net(sk), ifindex);
>> if (!dev)
>> return -ENXIO;
>>
>> @@ -714,7 +717,7 @@ void j1939sk_netdev_event(struct net_device
>> *netdev, int error_code)
>> j1939_name_local_put(priv, jsk->addr.src_name);
>> j1939_priv_put(priv);
>>
>> - j1939_netdev_stop(netdev);
>> + j1939_netdev_stop(sock_net(&jsk->sk), netdev);
>> }
>> /* do not remove filters here */
>> }
>> diff --git a/j1939/transport.c b/j1939/transport.c
>> index fbebcc20bccf..67722773c0b0 100644
>> --- a/j1939/transport.c
>> +++ b/j1939/transport.c
>> @@ -107,8 +107,8 @@ struct session {
>> /* forward declarations */
>> static struct session *j1939_session_new(struct sk_buff *skb);
>> static struct session *j1939_session_fresh_new(int size,
>> - struct sk_buff *rel_skb,
>> - pgn_t pgn);
>> + struct sk_buff *rel_skb,
>> + pgn_t pgn);
>> static void j1939tp_del_work(struct work_struct *work);
>
> This is not a namespace thing.
> Can you remove it.
> Maybe add another seperate patch if the whitespace is really better.
>
>>
>> /* local variables */
>> @@ -1152,7 +1152,7 @@ int j1939_send_transport(struct sk_buff *skb)
>> return ret;
>>
>> /* fix dst_flags, it may be used there soon */
>> - priv = j1939_priv_get_by_ifindex(can_skb_prv(skb)->ifindex);
>> + priv = j1939_priv_get_by_ifindex(dev_net(skb->dev),
>> can_skb_prv(skb)->ifindex);
>> if (!priv)
>> return -EINVAL;
>> if (j1939_address_is_unicast(cb->addr.da) &&
>> @@ -1273,8 +1273,8 @@ int j1939_recv_transport(struct sk_buff *skb)
>> }
>>
>> static struct session *j1939_session_fresh_new(int size,
>> - struct sk_buff *rel_skb,
>> - pgn_t pgn)
>> + struct sk_buff *rel_skb,
>> + pgn_t pgn)
>
> Only whitespace ...
>> {
>> struct sk_buff *skb;
>> struct j1939_sk_buff_cb *cb;
>> @@ -1400,6 +1400,7 @@ static struct ctl_table_header *sysctl_hdr;
>> /* module init */
>> int __init j1939tp_module_init(void)
>> {
>> + //TODO: not sure how to make this network namespace-aware??
>
> The question doesn't belong to the patch, but is valid.
> I don't have the answer. I suggest to make the j1939 sysctl
> not namespace-aware for a moment, and anyone who does not agree will
> know how to fix it?
>
>> sysctl_hdr = register_net_sysctl(&init_net, "net/can-j1939",
>> canj1939_sysctl_table);
>> if (!sysctl_hdr)
>>
>>
>> ---
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-06 20:16 ` Kurt Van Dijck
@ 2018-02-08 15:05 ` Marc Kleine-Budde
2018-02-08 21:13 ` Oliver Hartkopp
1 sibling, 0 replies; 12+ messages in thread
From: Marc Kleine-Budde @ 2018-02-08 15:05 UTC (permalink / raw)
To: Oliver Hartkopp, Elenita Hinds, linux-can
[-- Attachment #1.1: Type: text/plain, Size: 1221 bytes --]
On 02/06/2018 09:16 PM, Kurt Van Dijck wrote:
>> Elenita is right. The idea is to have different network namespaces (e.g. to
>> be used in (Docker-)containers) where each namespace has its own private
>> view to the network.
>
> So absolutely nothing is/should be shared between the namespaces
> from the userspace point of view?
>
> So the ideal j1939 implementation that shares nothing between
> interfaces only needs to forward an extra namespace parameter?
>
> How about ... /proc parameters? or Module parameters?
> or is that the main argument against them?
For now all proc related stuff is removed. There are two clean patches
to add things back or rather have them for reference in the j1939-proc
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=j1939-proc
> Can different namespaces re-use interface numbers (ifindex)?
>
> Anyway, I can start to take a look.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-07 16:41 ` Elenita Hinds
@ 2018-02-08 15:08 ` Marc Kleine-Budde
2018-02-08 16:26 ` Elenita Hinds
0 siblings, 1 reply; 12+ messages in thread
From: Marc Kleine-Budde @ 2018-02-08 15:08 UTC (permalink / raw)
To: Elenita Hinds, linux-can
[-- Attachment #1.1: Type: text/plain, Size: 1622 bytes --]
On 02/07/2018 05:41 PM, Elenita Hinds wrote:
> Thanks for the feedback, Kurt. I'll split into separate patches as
> suggested for clarity. The additional whitespaces were due to linux
> kernel styles imposed upon in my environment (enabled and won't let me
> commit unless I align certain lines).
I've manually picked the (maybe not all) whitespace changes manually and
added them to linux-can-next/j1939. Please send your patches using git
not via a webinterface, as it makes the patches unusable.
> I found missing changes in the j1939 notifier block which I think can
> cause the kernel to panic when a CAN bus interface goes down. This is
> fixed and will post the changes.
As Kurt said, please make this a seperate patch.
> I also figured out how to complete the TODO item and will post these as well.
>
> I'm also trying to coordinate these changes with Oleksij (Rempel) but
> he is out for the moment.
>
> To answer your questions above, based on the kernel examples and
> implied on the info I've seen, I think almost all need duplicating and
> this includes the /proc and module parameters (see detailed
> description in http://apprize.info/linux/kernel/15.html). And yes, the
> ifindex values can be re-used in different namespaces.
In the j1939 branch proc support has been removed for now.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-08 15:08 ` Marc Kleine-Budde
@ 2018-02-08 16:26 ` Elenita Hinds
0 siblings, 0 replies; 12+ messages in thread
From: Elenita Hinds @ 2018-02-08 16:26 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can
Thanks, Marc. It looks like I'm out of date with the latest j1939
branch by a couple of commits. Will be re-syncing.
On Thu, Feb 8, 2018 at 9:08 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 02/07/2018 05:41 PM, Elenita Hinds wrote:
>> Thanks for the feedback, Kurt. I'll split into separate patches as
>> suggested for clarity. The additional whitespaces were due to linux
>> kernel styles imposed upon in my environment (enabled and won't let me
>> commit unless I align certain lines).
>
> I've manually picked the (maybe not all) whitespace changes manually and
> added them to linux-can-next/j1939. Please send your patches using git
> not via a webinterface, as it makes the patches unusable.
>
>> I found missing changes in the j1939 notifier block which I think can
>> cause the kernel to panic when a CAN bus interface goes down. This is
>> fixed and will post the changes.
>
> As Kurt said, please make this a seperate patch.
>
>> I also figured out how to complete the TODO item and will post these as well.
>>
>> I'm also trying to coordinate these changes with Oleksij (Rempel) but
>> he is out for the moment.
>>
>> To answer your questions above, based on the kernel examples and
>> implied on the info I've seen, I think almost all need duplicating and
>> this includes the /proc and module parameters (see detailed
>> description in http://apprize.info/linux/kernel/15.html). And yes, the
>> ifindex values can be re-used in different namespaces.
>
> In the j1939 branch proc support has been removed for now.
>
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: j1939: Attempt to make j1939 network namespace-aware
2018-02-06 20:16 ` Kurt Van Dijck
2018-02-08 15:05 ` Marc Kleine-Budde
@ 2018-02-08 21:13 ` Oliver Hartkopp
1 sibling, 0 replies; 12+ messages in thread
From: Oliver Hartkopp @ 2018-02-08 21:13 UTC (permalink / raw)
To: Kurt Van Dijck; +Cc: Elenita Hinds, linux-can
Hi Kurt,
On 02/06/2018 09:16 PM, Kurt Van Dijck wrote:
> So absolutely nothing is/should be shared between the namespaces
> from the userspace point of view?
The network interfaces and the created sockets are only working and
visible in one specific namespace.
Before making the CAN subsystem namespace aware, it was only able to
work in the root aka init namespace.
When you have a single vcan0 in a namespace the application that
executes inside this namespace can only see this single interface.
> So the ideal j1939 implementation that shares nothing between
> interfaces only needs to forward an extra namespace parameter?
Yes. Each interface has a namespace and an ifindex. And each socket has
a namespace.
All relevant data structures need to be per-namespace.
The initial namespace support from Mario gives an idea about the changes
for the CAN subsystem (and CAN RAW):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/can?id=8e8cda6d737d356054c9eeef642aec0e8ae7e6bc
> How about ... /proc parameters?
per-namespace:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/can?id=cb5635a3677679666e4e81ecbb209d32f13dedcd
> or Module parameters?
Hm - we only have module_param(stats_timer, int, S_IRUGO) in af_can.c
This is just one module parameter for the module - which is not
application relevant (like addressing or filters).
> or is that the main argument against them?
Don't know. I think netlink and sockopts should cover most of the
configuration stuff. procfs and sysfs need to be namespace aware.
> Can different namespaces re-use interface numbers (ifindex)?
As written above every interface has a namespace and an ifindex.
So you may have the same ifindex value in different namespaces.
Best,
Oliver
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-02-08 21:13 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-05 20:31 j1939: Attempt to make j1939 network namespace-aware Elenita Hinds
2018-02-05 20:44 ` Kurt Van Dijck
2018-02-06 16:15 ` Elenita Hinds
2018-02-06 18:30 ` Oliver Hartkopp
2018-02-06 20:16 ` Kurt Van Dijck
2018-02-08 15:05 ` Marc Kleine-Budde
2018-02-08 21:13 ` Oliver Hartkopp
2018-02-06 8:52 ` Robert Schwebel
2018-02-06 20:37 ` Kurt Van Dijck
2018-02-07 16:41 ` Elenita Hinds
2018-02-08 15:08 ` Marc Kleine-Budde
2018-02-08 16:26 ` Elenita Hinds
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.