* [PATCH v2 0/2] can: Add CAN_RAW_RECV_OWN_MSGS_ALL socket option
@ 2021-05-04 20:35 Erik Flodin
2021-05-04 20:35 ` [PATCH v2 1/2] can: add support for filtering own messages only Erik Flodin
2021-05-04 20:35 ` [PATCH v2 2/2] can: raw: add CAN_RAW_RECV_OWN_MSGS_ALL socket option Erik Flodin
0 siblings, 2 replies; 13+ messages in thread
From: Erik Flodin @ 2021-05-04 20:35 UTC (permalink / raw)
To: socketcan, mkl
Cc: davem, kuba, corbet, linux-can, netdev, linux-doc, Erik Flodin
Add a socket option that works as CAN_RAW_RECV_OWN_MSGS but where reception
of a socket's own frame isn't subject to filtering. This way transmission
confirmation can more easily (or at all if CAN_RAW_JOIN_FILTERS is enabled)
be used in combination with filters.
Changes since v1:
- Rebase on top of can-next/master.
Erik Flodin (2):
can: add support for filtering own messages only
can: raw: add CAN_RAW_RECV_OWN_MSGS_ALL socket option
Documentation/networking/can.rst | 7 +++
include/linux/can/core.h | 4 +-
include/uapi/linux/can/raw.h | 18 +++---
net/can/af_can.c | 50 ++++++++-------
net/can/af_can.h | 1 +
net/can/bcm.c | 9 ++-
net/can/gw.c | 7 ++-
net/can/isotp.c | 8 +--
net/can/j1939/main.c | 4 +-
net/can/proc.c | 11 ++--
net/can/raw.c | 101 +++++++++++++++++++++++++------
11 files changed, 153 insertions(+), 67 deletions(-)
base-commit: 9d31d2338950293ec19d9b095fbaa9030899dcb4
--
2.31.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-04 20:35 [PATCH v2 0/2] can: Add CAN_RAW_RECV_OWN_MSGS_ALL socket option Erik Flodin
@ 2021-05-04 20:35 ` Erik Flodin
2021-05-05 6:58 ` Oliver Hartkopp
2021-05-04 20:35 ` [PATCH v2 2/2] can: raw: add CAN_RAW_RECV_OWN_MSGS_ALL socket option Erik Flodin
1 sibling, 1 reply; 13+ messages in thread
From: Erik Flodin @ 2021-05-04 20:35 UTC (permalink / raw)
To: socketcan, mkl
Cc: davem, kuba, corbet, linux-can, netdev, linux-doc, Erik Flodin
Add a new flag to can_rx_register/unregister that, when set, requires
that the received sk_buff's owning socket matches the socket pointer
given when setting the filter. This makes it possible to set a filter
that matches all frames sent on a given socket and nothing else.
Signed-off-by: Erik Flodin <erik@flodin.me>
---
include/linux/can/core.h | 4 ++--
net/can/af_can.c | 50 ++++++++++++++++++++++------------------
net/can/af_can.h | 1 +
net/can/bcm.c | 9 +++++---
net/can/gw.c | 7 +++---
net/can/isotp.c | 8 +++----
net/can/j1939/main.c | 4 ++--
net/can/proc.c | 11 +++++----
net/can/raw.c | 10 ++++----
9 files changed, 58 insertions(+), 46 deletions(-)
diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 5fb8d0e3f9c1..7ee68128dc10 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -48,12 +48,12 @@ extern int can_proto_register(const struct can_proto *cp);
extern void can_proto_unregister(const struct can_proto *cp);
int can_rx_register(struct net *net, struct net_device *dev,
- canid_t can_id, canid_t mask,
+ canid_t can_id, canid_t mask, bool match_sk,
void (*func)(struct sk_buff *, void *),
void *data, char *ident, struct sock *sk);
extern void can_rx_unregister(struct net *net, struct net_device *dev,
- canid_t can_id, canid_t mask,
+ canid_t can_id, canid_t mask, bool match_sk,
void (*func)(struct sk_buff *, void *),
void *data);
diff --git a/net/can/af_can.c b/net/can/af_can.c
index cce2af10eb3e..7b639c121653 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -414,6 +414,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
* @dev: pointer to netdevice (NULL => subscribe from 'all' CAN devices list)
* @can_id: CAN identifier (see description)
* @mask: CAN mask (see description)
+ * @match_sk: match socket pointer on received sk_buff (see description)
* @func: callback function on filter match
* @data: returned parameter for callback function
* @ident: string for calling module identification
@@ -428,6 +429,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
* The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can
* filter for error message frames (CAN_ERR_FLAG bit set in mask).
*
+ * If match_sk is true, the received sk_buff's owning socket must also match
+ * the given socket pointer.
+ *
* The provided pointer to the sk_buff is guaranteed to be valid as long as
* the callback function is running. The callback function must *not* free
* the given sk_buff while processing it's task. When the given sk_buff is
@@ -440,8 +444,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
* -ENODEV unknown device
*/
int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
- canid_t mask, void (*func)(struct sk_buff *, void *),
- void *data, char *ident, struct sock *sk)
+ canid_t mask, bool match_sk,
+ void (*func)(struct sk_buff *, void *), void *data,
+ char *ident, struct sock *sk)
{
struct receiver *rcv;
struct hlist_head *rcv_list;
@@ -468,6 +473,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
rcv->can_id = can_id;
rcv->mask = mask;
+ rcv->match_sk = match_sk;
rcv->matches = 0;
rcv->func = func;
rcv->data = data;
@@ -503,6 +509,7 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
* @dev: pointer to netdevice (NULL => unsubscribe from 'all' CAN devices list)
* @can_id: CAN identifier
* @mask: CAN mask
+ * @match_sk: match socket pointer on received sk_buff
* @func: callback function on filter match
* @data: returned parameter for callback function
*
@@ -510,8 +517,8 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
* Removes subscription entry depending on given (subscription) values.
*/
void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
- canid_t mask, void (*func)(struct sk_buff *, void *),
- void *data)
+ canid_t mask, bool match_sk,
+ void (*func)(struct sk_buff *, void *), void *data)
{
struct receiver *rcv = NULL;
struct hlist_head *rcv_list;
@@ -535,7 +542,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
*/
hlist_for_each_entry_rcu(rcv, rcv_list, list) {
if (rcv->can_id == can_id && rcv->mask == mask &&
- rcv->func == func && rcv->data == data)
+ rcv->match_sk == match_sk && rcv->func == func &&
+ rcv->data == data)
break;
}
@@ -546,8 +554,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
* a warning here.
*/
if (!rcv) {
- pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
- DNAME(dev), can_id, mask);
+ pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X%s\n",
+ DNAME(dev), can_id, mask, match_sk ? " (match sk)" : "");
goto out;
}
@@ -569,10 +577,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
}
EXPORT_SYMBOL(can_rx_unregister);
-static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
+static inline int deliver(struct sk_buff *skb, struct receiver *rcv)
{
- rcv->func(skb, rcv->data);
- rcv->matches++;
+ if (!rcv->match_sk || skb->sk == rcv->sk) {
+ rcv->func(skb, rcv->data);
+ rcv->matches++;
+ return 1;
+ }
+ return 0;
}
static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buff *skb)
@@ -589,8 +601,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
/* check for error message frame entries only */
hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ERR], list) {
if (can_id & rcv->mask) {
- deliver(skb, rcv);
- matches++;
+ matches += deliver(skb, rcv);
}
}
return matches;
@@ -598,23 +609,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
/* check for unfiltered entries */
hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ALL], list) {
- deliver(skb, rcv);
- matches++;
+ matches += deliver(skb, rcv);
}
/* check for can_id/mask entries */
hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_FIL], list) {
if ((can_id & rcv->mask) == rcv->can_id) {
- deliver(skb, rcv);
- matches++;
+ matches += deliver(skb, rcv);
}
}
/* check for inverted can_id/mask entries */
hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_INV], list) {
if ((can_id & rcv->mask) != rcv->can_id) {
- deliver(skb, rcv);
- matches++;
+ matches += deliver(skb, rcv);
}
}
@@ -625,15 +633,13 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
if (can_id & CAN_EFF_FLAG) {
hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_eff[effhash(can_id)], list) {
if (rcv->can_id == can_id) {
- deliver(skb, rcv);
- matches++;
+ matches += deliver(skb, rcv);
}
}
} else {
can_id &= CAN_SFF_MASK;
hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_sff[can_id], list) {
- deliver(skb, rcv);
- matches++;
+ matches += deliver(skb, rcv);
}
}
diff --git a/net/can/af_can.h b/net/can/af_can.h
index 7c2d9161e224..ea98b10d93e7 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -52,6 +52,7 @@ struct receiver {
struct hlist_node list;
canid_t can_id;
canid_t mask;
+ bool match_sk;
unsigned long matches;
void (*func)(struct sk_buff *skb, void *data);
void *data;
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 909b9e684e04..d89dd82c2178 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -729,7 +729,8 @@ static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
{
if (op->rx_reg_dev == dev) {
can_rx_unregister(dev_net(dev), dev, op->can_id,
- REGMASK(op->can_id), bcm_rx_handler, op);
+ REGMASK(op->can_id), false, bcm_rx_handler,
+ op);
/* mark as removed subscription */
op->rx_reg_dev = NULL;
@@ -775,6 +776,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
can_rx_unregister(sock_net(op->sk), NULL,
op->can_id,
REGMASK(op->can_id),
+ false,
bcm_rx_handler, op);
list_del(&op->list);
@@ -1193,6 +1195,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
err = can_rx_register(sock_net(sk), dev,
op->can_id,
REGMASK(op->can_id),
+ false,
bcm_rx_handler, op,
"bcm", sk);
@@ -1202,7 +1205,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
} else
err = can_rx_register(sock_net(sk), NULL, op->can_id,
- REGMASK(op->can_id),
+ REGMASK(op->can_id), false,
bcm_rx_handler, op, "bcm", sk);
if (err) {
/* this bcm rx op is broken -> remove it */
@@ -1500,7 +1503,7 @@ static int bcm_release(struct socket *sock)
}
} else
can_rx_unregister(net, NULL, op->can_id,
- REGMASK(op->can_id),
+ REGMASK(op->can_id), false,
bcm_rx_handler, op);
bcm_remove_op(op);
diff --git a/net/can/gw.c b/net/can/gw.c
index ba4124805602..5dbc7b85e0fc 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -567,14 +567,15 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
{
return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id,
- gwj->ccgw.filter.can_mask, can_can_gw_rcv,
- gwj, "gw", NULL);
+ gwj->ccgw.filter.can_mask, false,
+ can_can_gw_rcv, gwj, "gw", NULL);
}
static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
{
can_rx_unregister(net, gwj->src.dev, gwj->ccgw.filter.can_id,
- gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
+ gwj->ccgw.filter.can_mask, false, can_can_gw_rcv,
+ gwj);
}
static int cgw_notifier(struct notifier_block *nb,
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 9f94ad3caee9..44d943bbe0b1 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1029,7 +1029,7 @@ static int isotp_release(struct socket *sock)
if (dev) {
can_rx_unregister(net, dev, so->rxid,
SINGLE_MASK(so->rxid),
- isotp_rcv, sk);
+ false, isotp_rcv, sk);
dev_put(dev);
}
}
@@ -1111,7 +1111,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
if (do_rx_reg)
can_rx_register(net, dev, addr->can_addr.tp.rx_id,
SINGLE_MASK(addr->can_addr.tp.rx_id),
- isotp_rcv, sk, "isotp", sk);
+ false, isotp_rcv, sk, "isotp", sk);
dev_put(dev);
@@ -1122,7 +1122,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
if (dev) {
can_rx_unregister(net, dev, so->rxid,
SINGLE_MASK(so->rxid),
- isotp_rcv, sk);
+ false, isotp_rcv, sk);
dev_put(dev);
}
}
@@ -1323,7 +1323,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST)))
can_rx_unregister(dev_net(dev), dev, so->rxid,
SINGLE_MASK(so->rxid),
- isotp_rcv, sk);
+ false, isotp_rcv, sk);
so->ifindex = 0;
so->bound = 0;
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index da3a7a7bcff2..466a20c76fb6 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -177,7 +177,7 @@ static int j1939_can_rx_register(struct j1939_priv *priv)
j1939_priv_get(priv);
ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
- j1939_can_recv, priv, "j1939", NULL);
+ false, j1939_can_recv, priv, "j1939", NULL);
if (ret < 0) {
j1939_priv_put(priv);
return ret;
@@ -191,7 +191,7 @@ static void j1939_can_rx_unregister(struct j1939_priv *priv)
struct net_device *ndev = priv->ndev;
can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
- j1939_can_recv, priv);
+ false, j1939_can_recv, priv);
j1939_priv_put(priv);
}
diff --git a/net/can/proc.c b/net/can/proc.c
index d1fe49e6f16d..d312077832a6 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -191,11 +191,12 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
hlist_for_each_entry_rcu(r, rx_list, list) {
char *fmt = (r->can_id & CAN_EFF_FLAG)?
- " %-5s %08x %08x %pK %pK %8ld %s\n" :
- " %-5s %03x %08x %pK %pK %8ld %s\n";
+ " %-5s %08x %08x %c %pK %pK %8ld %s\n" :
+ " %-5s %03x %08x %c %pK %pK %8ld %s\n";
seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
- r->func, r->data, r->matches, r->ident);
+ r->match_sk ? '*' : ' ', r->func, r->data,
+ r->matches, r->ident);
}
}
@@ -206,9 +207,9 @@ static void can_print_recv_banner(struct seq_file *m)
* ....... 0 tp20
*/
if (IS_ENABLED(CONFIG_64BIT))
- seq_puts(m, " device can_id can_mask function userdata matches ident\n");
+ seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
else
- seq_puts(m, " device can_id can_mask function userdata matches ident\n");
+ seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
}
static int can_stats_proc_show(struct seq_file *m, void *v)
diff --git a/net/can/raw.c b/net/can/raw.c
index 139d9471ddcf..acfbae28d451 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -187,13 +187,13 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
for (i = 0; i < count; i++) {
err = can_rx_register(net, dev, filter[i].can_id,
- filter[i].can_mask,
+ filter[i].can_mask, false,
raw_rcv, sk, "raw", sk);
if (err) {
/* clean up successfully registered filters */
while (--i >= 0)
can_rx_unregister(net, dev, filter[i].can_id,
- filter[i].can_mask,
+ filter[i].can_mask, false,
raw_rcv, sk);
break;
}
@@ -209,7 +209,7 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev,
if (err_mask)
err = can_rx_register(net, dev, 0, err_mask | CAN_ERR_FLAG,
- raw_rcv, sk, "raw", sk);
+ false, raw_rcv, sk, "raw", sk);
return err;
}
@@ -222,7 +222,7 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
for (i = 0; i < count; i++)
can_rx_unregister(net, dev, filter[i].can_id,
- filter[i].can_mask, raw_rcv, sk);
+ filter[i].can_mask, false, raw_rcv, sk);
}
static inline void raw_disable_errfilter(struct net *net,
@@ -233,7 +233,7 @@ static inline void raw_disable_errfilter(struct net *net,
{
if (err_mask)
can_rx_unregister(net, dev, 0, err_mask | CAN_ERR_FLAG,
- raw_rcv, sk);
+ false, raw_rcv, sk);
}
static inline void raw_disable_allfilters(struct net *net,
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] can: raw: add CAN_RAW_RECV_OWN_MSGS_ALL socket option
2021-05-04 20:35 [PATCH v2 0/2] can: Add CAN_RAW_RECV_OWN_MSGS_ALL socket option Erik Flodin
2021-05-04 20:35 ` [PATCH v2 1/2] can: add support for filtering own messages only Erik Flodin
@ 2021-05-04 20:35 ` Erik Flodin
1 sibling, 0 replies; 13+ messages in thread
From: Erik Flodin @ 2021-05-04 20:35 UTC (permalink / raw)
To: socketcan, mkl
Cc: davem, kuba, corbet, linux-can, netdev, linux-doc, Erik Flodin
CAN_RAW_RECV_OWN_MSGS_ALL works as CAN_RAW_RECV_OWN_MSGS with the
difference that all sent frames are received as no filtering is applied
on the socket's own frames in this case.
Signed-off-by: Erik Flodin <erik@flodin.me>
---
Documentation/networking/can.rst | 7 +++
include/uapi/linux/can/raw.h | 18 ++++---
net/can/raw.c | 91 +++++++++++++++++++++++++++-----
3 files changed, 95 insertions(+), 21 deletions(-)
diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
index f34cb0e4460e..80c70357cd33 100644
--- a/Documentation/networking/can.rst
+++ b/Documentation/networking/can.rst
@@ -611,6 +611,13 @@ demand:
Note that reception of a socket's own CAN frames are subject to the same
filtering as other CAN frames (see :ref:`socketcan-rawfilter`).
+RAW socket option CAN_RAW_RECV_OWN_MSGS_ALL
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Identical to CAN_RAW_RECV_OWN_MSGS except that all sent messages are
+received. I.e. reception is not subject to filtering.
+
+
.. _socketcan-rawfd:
RAW Socket Option CAN_RAW_FD_FRAMES
diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index 3386aa81fdf2..6e29b2b145e2 100644
--- a/include/uapi/linux/can/raw.h
+++ b/include/uapi/linux/can/raw.h
@@ -53,15 +53,17 @@ enum {
SCM_CAN_RAW_ERRQUEUE = 1,
};
-/* for socket options affecting the socket (not the global system) */
-
+/* For socket options affecting the socket (not the global system).
+ * Options default to off unless noted otherwise.
+ */
enum {
- CAN_RAW_FILTER = 1, /* set 0 .. n can_filter(s) */
- CAN_RAW_ERR_FILTER, /* set filter for error frames */
- CAN_RAW_LOOPBACK, /* local loopback (default:on) */
- CAN_RAW_RECV_OWN_MSGS, /* receive my own msgs (default:off) */
- CAN_RAW_FD_FRAMES, /* allow CAN FD frames (default:off) */
- CAN_RAW_JOIN_FILTERS, /* all filters must match to trigger */
+ CAN_RAW_FILTER = 1, /* set 0 .. n can_filter(s) */
+ CAN_RAW_ERR_FILTER, /* set filter for error frames */
+ CAN_RAW_LOOPBACK, /* local loopback (default on) */
+ CAN_RAW_RECV_OWN_MSGS, /* receive my own msgs w/ filtering */
+ CAN_RAW_FD_FRAMES, /* allow CAN FD frames */
+ CAN_RAW_JOIN_FILTERS, /* all filters must match to trigger */
+ CAN_RAW_RECV_OWN_MSGS_ALL, /* receive my own msgs w/o filtering */
};
#endif /* !_UAPI_CAN_RAW_H */
diff --git a/net/can/raw.c b/net/can/raw.c
index acfbae28d451..79c29942b0be 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -86,6 +86,7 @@ struct raw_sock {
struct notifier_block notifier;
int loopback;
int recv_own_msgs;
+ int recv_own_msgs_all;
int fd_frames;
int join_filters;
int count; /* number of active filters */
@@ -122,7 +123,7 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
unsigned int *pflags;
/* check the received tx sock reference */
- if (!ro->recv_own_msgs && oskb->sk == sk)
+ if (!ro->recv_own_msgs && !ro->recv_own_msgs_all && oskb->sk == sk)
return;
/* do not pass non-CAN2.0 frames to a legacy socket */
@@ -132,7 +133,8 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
/* eliminate multiple filter matches for the same skb */
if (this_cpu_ptr(ro->uniq)->skb == oskb &&
this_cpu_ptr(ro->uniq)->skbcnt == can_skb_prv(oskb)->skbcnt) {
- if (ro->join_filters) {
+ if (ro->join_filters &&
+ (!ro->recv_own_msgs_all || oskb->sk != sk)) {
this_cpu_inc(ro->uniq->join_rx_count);
/* drop frame until all enabled filters matched */
if (this_cpu_ptr(ro->uniq)->join_rx_count < ro->count)
@@ -145,8 +147,10 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
this_cpu_ptr(ro->uniq)->skbcnt = can_skb_prv(oskb)->skbcnt;
this_cpu_ptr(ro->uniq)->join_rx_count = 1;
/* drop first frame to check all enabled filters? */
- if (ro->join_filters && ro->count > 1)
+ if (ro->join_filters && ro->count > 1 &&
+ (!ro->recv_own_msgs_all || oskb->sk != sk)) {
return;
+ }
}
/* clone the given skb to be able to enqueue it into the rcv queue */
@@ -214,6 +218,18 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev,
return err;
}
+static int raw_enable_ownfilter(struct net *net, struct net_device *dev,
+ struct sock *sk, bool recv_own_msgs_all)
+{
+ int err = 0;
+
+ if (recv_own_msgs_all)
+ err = can_rx_register(net, dev, 0, MASK_ALL, true, raw_rcv,
+ sk, "raw", sk);
+
+ return err;
+}
+
static void raw_disable_filters(struct net *net, struct net_device *dev,
struct sock *sk, struct can_filter *filter,
int count)
@@ -236,6 +252,13 @@ static inline void raw_disable_errfilter(struct net *net,
false, raw_rcv, sk);
}
+static void raw_disable_ownfilter(struct net *net, struct net_device *dev,
+ struct sock *sk, bool recv_own_msgs_all)
+{
+ if (recv_own_msgs_all)
+ can_rx_unregister(net, dev, 0, MASK_ALL, true, raw_rcv, sk);
+}
+
static inline void raw_disable_allfilters(struct net *net,
struct net_device *dev,
struct sock *sk)
@@ -244,6 +267,7 @@ static inline void raw_disable_allfilters(struct net *net,
raw_disable_filters(net, dev, sk, ro->filter, ro->count);
raw_disable_errfilter(net, dev, sk, ro->err_mask);
+ raw_disable_ownfilter(net, dev, sk, ro->recv_own_msgs_all);
}
static int raw_enable_allfilters(struct net *net, struct net_device *dev,
@@ -253,13 +277,19 @@ static int raw_enable_allfilters(struct net *net, struct net_device *dev,
int err;
err = raw_enable_filters(net, dev, sk, ro->filter, ro->count);
- if (!err) {
- err = raw_enable_errfilter(net, dev, sk, ro->err_mask);
- if (err)
- raw_disable_filters(net, dev, sk, ro->filter,
- ro->count);
- }
+ if (err)
+ goto out;
+ err = raw_enable_errfilter(net, dev, sk, ro->err_mask);
+ if (err)
+ goto out_disable;
+ err = raw_enable_ownfilter(net, dev, sk, ro->recv_own_msgs_all);
+ if (!err)
+ goto out;
+ raw_disable_errfilter(net, dev, sk, ro->err_mask);
+out_disable:
+ raw_disable_filters(net, dev, sk, ro->filter, ro->count);
+out:
return err;
}
@@ -323,10 +353,11 @@ static int raw_init(struct sock *sk)
ro->count = 1;
/* set default loopback behaviour */
- ro->loopback = 1;
- ro->recv_own_msgs = 0;
- ro->fd_frames = 0;
- ro->join_filters = 0;
+ ro->loopback = 1;
+ ro->recv_own_msgs = 0;
+ ro->recv_own_msgs_all = 0;
+ ro->fd_frames = 0;
+ ro->join_filters = 0;
/* alloc_percpu provides zero'ed memory */
ro->uniq = alloc_percpu(struct uniqframe);
@@ -495,6 +526,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
can_err_mask_t err_mask = 0;
int count = 0;
int err = 0;
+ int old_val;
if (level != SOL_CAN_RAW)
return -EINVAL;
@@ -639,6 +671,33 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
break;
+ case CAN_RAW_RECV_OWN_MSGS_ALL:
+ if (optlen != sizeof(ro->recv_own_msgs_all))
+ return -EINVAL;
+
+ old_val = ro->recv_own_msgs_all;
+ if (copy_from_sockptr(&ro->recv_own_msgs_all, optval, optlen))
+ return -EFAULT;
+
+ lock_sock(sk);
+
+ if (ro->bound && ro->ifindex)
+ dev = dev_get_by_index(sock_net(sk), ro->ifindex);
+
+ if (ro->bound) {
+ if (old_val && !ro->recv_own_msgs_all)
+ raw_disable_ownfilter(sock_net(sk), dev, sk, true);
+ else if (!old_val && ro->recv_own_msgs_all)
+ err = raw_enable_ownfilter(sock_net(sk), dev, sk, true);
+ }
+
+ if (dev)
+ dev_put(dev);
+
+ release_sock(sk);
+
+ break;
+
default:
return -ENOPROTOOPT;
}
@@ -718,6 +777,12 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
val = &ro->join_filters;
break;
+ case CAN_RAW_RECV_OWN_MSGS_ALL:
+ if (len > sizeof(int))
+ len = sizeof(int);
+ val = &ro->recv_own_msgs_all;
+ break;
+
default:
return -ENOPROTOOPT;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-04 20:35 ` [PATCH v2 1/2] can: add support for filtering own messages only Erik Flodin
@ 2021-05-05 6:58 ` Oliver Hartkopp
2021-05-05 8:37 ` Erik Flodin
0 siblings, 1 reply; 13+ messages in thread
From: Oliver Hartkopp @ 2021-05-05 6:58 UTC (permalink / raw)
To: Erik Flodin, mkl; +Cc: davem, kuba, corbet, linux-can, netdev, linux-doc
Hi Erik,
IMO putting linux-can@vger.kernel.org in CC would be sufficient in this
stage of discussion.
On 04.05.21 22:35, Erik Flodin wrote:
> Add a new flag to can_rx_register/unregister that, when set, requires
> that the received sk_buff's owning socket matches the socket pointer
> given when setting the filter. This makes it possible to set a filter
> that matches all frames sent on a given socket and nothing else.
This is a pretty simple requirement and you are touching almost every
CAN network layer file.
From this description I would suggest a two-liner to be added in raw.c
diff --git a/net/can/raw.c b/net/can/raw.c
index 139d9471ddcf..8c6371a0c8a6 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -123,10 +123,14 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
/* check the received tx sock reference */
if (!ro->recv_own_msgs && oskb->sk == sk)
return;
+ /* check the received tx sock reference */
+ if (ro->recv_only_own_msgs && oskb->sk != sk)
+ return;
+
/* do not pass non-CAN2.0 frames to a legacy socket */
if (!ro->fd_frames && oskb->len != CAN_MTU)
return;
/* eliminate multiple filter matches for the same skb */
And then add CAN_RAW_RECV_ONLY_OWN_MSGS accordingly.
Wouldn't that already match your requirements?
Best regards,
Oliver
ps. please reduce the CC receipient list before answering, thanks.
>
> Signed-off-by: Erik Flodin <erik@flodin.me>
> ---
> include/linux/can/core.h | 4 ++--
> net/can/af_can.c | 50 ++++++++++++++++++++++------------------
> net/can/af_can.h | 1 +
> net/can/bcm.c | 9 +++++---
> net/can/gw.c | 7 +++---
> net/can/isotp.c | 8 +++----
> net/can/j1939/main.c | 4 ++--
> net/can/proc.c | 11 +++++----
> net/can/raw.c | 10 ++++----
> 9 files changed, 58 insertions(+), 46 deletions(-)
>
> diff --git a/include/linux/can/core.h b/include/linux/can/core.h
> index 5fb8d0e3f9c1..7ee68128dc10 100644
> --- a/include/linux/can/core.h
> +++ b/include/linux/can/core.h
> @@ -48,12 +48,12 @@ extern int can_proto_register(const struct can_proto *cp);
> extern void can_proto_unregister(const struct can_proto *cp);
>
> int can_rx_register(struct net *net, struct net_device *dev,
> - canid_t can_id, canid_t mask,
> + canid_t can_id, canid_t mask, bool match_sk,
> void (*func)(struct sk_buff *, void *),
> void *data, char *ident, struct sock *sk);
>
> extern void can_rx_unregister(struct net *net, struct net_device *dev,
> - canid_t can_id, canid_t mask,
> + canid_t can_id, canid_t mask, bool match_sk,
> void (*func)(struct sk_buff *, void *),
> void *data);
>
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index cce2af10eb3e..7b639c121653 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -414,6 +414,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
> * @dev: pointer to netdevice (NULL => subscribe from 'all' CAN devices list)
> * @can_id: CAN identifier (see description)
> * @mask: CAN mask (see description)
> + * @match_sk: match socket pointer on received sk_buff (see description)
> * @func: callback function on filter match
> * @data: returned parameter for callback function
> * @ident: string for calling module identification
> @@ -428,6 +429,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
> * The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can
> * filter for error message frames (CAN_ERR_FLAG bit set in mask).
> *
> + * If match_sk is true, the received sk_buff's owning socket must also match
> + * the given socket pointer.
> + *
> * The provided pointer to the sk_buff is guaranteed to be valid as long as
> * the callback function is running. The callback function must *not* free
> * the given sk_buff while processing it's task. When the given sk_buff is
> @@ -440,8 +444,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
> * -ENODEV unknown device
> */
> int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
> - canid_t mask, void (*func)(struct sk_buff *, void *),
> - void *data, char *ident, struct sock *sk)
> + canid_t mask, bool match_sk,
> + void (*func)(struct sk_buff *, void *), void *data,
> + char *ident, struct sock *sk)
> {
> struct receiver *rcv;
> struct hlist_head *rcv_list;
> @@ -468,6 +473,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>
> rcv->can_id = can_id;
> rcv->mask = mask;
> + rcv->match_sk = match_sk;
> rcv->matches = 0;
> rcv->func = func;
> rcv->data = data;
> @@ -503,6 +509,7 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
> * @dev: pointer to netdevice (NULL => unsubscribe from 'all' CAN devices list)
> * @can_id: CAN identifier
> * @mask: CAN mask
> + * @match_sk: match socket pointer on received sk_buff
> * @func: callback function on filter match
> * @data: returned parameter for callback function
> *
> @@ -510,8 +517,8 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
> * Removes subscription entry depending on given (subscription) values.
> */
> void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> - canid_t mask, void (*func)(struct sk_buff *, void *),
> - void *data)
> + canid_t mask, bool match_sk,
> + void (*func)(struct sk_buff *, void *), void *data)
> {
> struct receiver *rcv = NULL;
> struct hlist_head *rcv_list;
> @@ -535,7 +542,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> */
> hlist_for_each_entry_rcu(rcv, rcv_list, list) {
> if (rcv->can_id == can_id && rcv->mask == mask &&
> - rcv->func == func && rcv->data == data)
> + rcv->match_sk == match_sk && rcv->func == func &&
> + rcv->data == data)
> break;
> }
>
> @@ -546,8 +554,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> * a warning here.
> */
> if (!rcv) {
> - pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
> - DNAME(dev), can_id, mask);
> + pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X%s\n",
> + DNAME(dev), can_id, mask, match_sk ? " (match sk)" : "");
> goto out;
> }
>
> @@ -569,10 +577,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> }
> EXPORT_SYMBOL(can_rx_unregister);
>
> -static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
> +static inline int deliver(struct sk_buff *skb, struct receiver *rcv)
> {
> - rcv->func(skb, rcv->data);
> - rcv->matches++;
> + if (!rcv->match_sk || skb->sk == rcv->sk) {
> + rcv->func(skb, rcv->data);
> + rcv->matches++;
> + return 1;
> + }
> + return 0;
> }
>
> static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buff *skb)
> @@ -589,8 +601,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
> /* check for error message frame entries only */
> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ERR], list) {
> if (can_id & rcv->mask) {
> - deliver(skb, rcv);
> - matches++;
> + matches += deliver(skb, rcv);
> }
> }
> return matches;
> @@ -598,23 +609,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
>
> /* check for unfiltered entries */
> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ALL], list) {
> - deliver(skb, rcv);
> - matches++;
> + matches += deliver(skb, rcv);
> }
>
> /* check for can_id/mask entries */
> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_FIL], list) {
> if ((can_id & rcv->mask) == rcv->can_id) {
> - deliver(skb, rcv);
> - matches++;
> + matches += deliver(skb, rcv);
> }
> }
>
> /* check for inverted can_id/mask entries */
> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_INV], list) {
> if ((can_id & rcv->mask) != rcv->can_id) {
> - deliver(skb, rcv);
> - matches++;
> + matches += deliver(skb, rcv);
> }
> }
>
> @@ -625,15 +633,13 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
> if (can_id & CAN_EFF_FLAG) {
> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_eff[effhash(can_id)], list) {
> if (rcv->can_id == can_id) {
> - deliver(skb, rcv);
> - matches++;
> + matches += deliver(skb, rcv);
> }
> }
> } else {
> can_id &= CAN_SFF_MASK;
> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_sff[can_id], list) {
> - deliver(skb, rcv);
> - matches++;
> + matches += deliver(skb, rcv);
> }
> }
>
> diff --git a/net/can/af_can.h b/net/can/af_can.h
> index 7c2d9161e224..ea98b10d93e7 100644
> --- a/net/can/af_can.h
> +++ b/net/can/af_can.h
> @@ -52,6 +52,7 @@ struct receiver {
> struct hlist_node list;
> canid_t can_id;
> canid_t mask;
> + bool match_sk;
> unsigned long matches;
> void (*func)(struct sk_buff *skb, void *data);
> void *data;
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 909b9e684e04..d89dd82c2178 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -729,7 +729,8 @@ static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
> {
> if (op->rx_reg_dev == dev) {
> can_rx_unregister(dev_net(dev), dev, op->can_id,
> - REGMASK(op->can_id), bcm_rx_handler, op);
> + REGMASK(op->can_id), false, bcm_rx_handler,
> + op);
>
> /* mark as removed subscription */
> op->rx_reg_dev = NULL;
> @@ -775,6 +776,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
> can_rx_unregister(sock_net(op->sk), NULL,
> op->can_id,
> REGMASK(op->can_id),
> + false,
> bcm_rx_handler, op);
>
> list_del(&op->list);
> @@ -1193,6 +1195,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> err = can_rx_register(sock_net(sk), dev,
> op->can_id,
> REGMASK(op->can_id),
> + false,
> bcm_rx_handler, op,
> "bcm", sk);
>
> @@ -1202,7 +1205,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>
> } else
> err = can_rx_register(sock_net(sk), NULL, op->can_id,
> - REGMASK(op->can_id),
> + REGMASK(op->can_id), false,
> bcm_rx_handler, op, "bcm", sk);
> if (err) {
> /* this bcm rx op is broken -> remove it */
> @@ -1500,7 +1503,7 @@ static int bcm_release(struct socket *sock)
> }
> } else
> can_rx_unregister(net, NULL, op->can_id,
> - REGMASK(op->can_id),
> + REGMASK(op->can_id), false,
> bcm_rx_handler, op);
>
> bcm_remove_op(op);
> diff --git a/net/can/gw.c b/net/can/gw.c
> index ba4124805602..5dbc7b85e0fc 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -567,14 +567,15 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
> static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
> {
> return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id,
> - gwj->ccgw.filter.can_mask, can_can_gw_rcv,
> - gwj, "gw", NULL);
> + gwj->ccgw.filter.can_mask, false,
> + can_can_gw_rcv, gwj, "gw", NULL);
> }
>
> static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
> {
> can_rx_unregister(net, gwj->src.dev, gwj->ccgw.filter.can_id,
> - gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
> + gwj->ccgw.filter.can_mask, false, can_can_gw_rcv,
> + gwj);
> }
>
> static int cgw_notifier(struct notifier_block *nb,
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 9f94ad3caee9..44d943bbe0b1 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1029,7 +1029,7 @@ static int isotp_release(struct socket *sock)
> if (dev) {
> can_rx_unregister(net, dev, so->rxid,
> SINGLE_MASK(so->rxid),
> - isotp_rcv, sk);
> + false, isotp_rcv, sk);
> dev_put(dev);
> }
> }
> @@ -1111,7 +1111,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> if (do_rx_reg)
> can_rx_register(net, dev, addr->can_addr.tp.rx_id,
> SINGLE_MASK(addr->can_addr.tp.rx_id),
> - isotp_rcv, sk, "isotp", sk);
> + false, isotp_rcv, sk, "isotp", sk);
>
> dev_put(dev);
>
> @@ -1122,7 +1122,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> if (dev) {
> can_rx_unregister(net, dev, so->rxid,
> SINGLE_MASK(so->rxid),
> - isotp_rcv, sk);
> + false, isotp_rcv, sk);
> dev_put(dev);
> }
> }
> @@ -1323,7 +1323,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
> if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST)))
> can_rx_unregister(dev_net(dev), dev, so->rxid,
> SINGLE_MASK(so->rxid),
> - isotp_rcv, sk);
> + false, isotp_rcv, sk);
>
> so->ifindex = 0;
> so->bound = 0;
> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index da3a7a7bcff2..466a20c76fb6 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
> @@ -177,7 +177,7 @@ static int j1939_can_rx_register(struct j1939_priv *priv)
>
> j1939_priv_get(priv);
> ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> - j1939_can_recv, priv, "j1939", NULL);
> + false, j1939_can_recv, priv, "j1939", NULL);
> if (ret < 0) {
> j1939_priv_put(priv);
> return ret;
> @@ -191,7 +191,7 @@ static void j1939_can_rx_unregister(struct j1939_priv *priv)
> struct net_device *ndev = priv->ndev;
>
> can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> - j1939_can_recv, priv);
> + false, j1939_can_recv, priv);
>
> j1939_priv_put(priv);
> }
> diff --git a/net/can/proc.c b/net/can/proc.c
> index d1fe49e6f16d..d312077832a6 100644
> --- a/net/can/proc.c
> +++ b/net/can/proc.c
> @@ -191,11 +191,12 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
>
> hlist_for_each_entry_rcu(r, rx_list, list) {
> char *fmt = (r->can_id & CAN_EFF_FLAG)?
> - " %-5s %08x %08x %pK %pK %8ld %s\n" :
> - " %-5s %03x %08x %pK %pK %8ld %s\n";
> + " %-5s %08x %08x %c %pK %pK %8ld %s\n" :
> + " %-5s %03x %08x %c %pK %pK %8ld %s\n";
>
> seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
> - r->func, r->data, r->matches, r->ident);
> + r->match_sk ? '*' : ' ', r->func, r->data,
> + r->matches, r->ident);
> }
> }
>
> @@ -206,9 +207,9 @@ static void can_print_recv_banner(struct seq_file *m)
> * ....... 0 tp20
> */
> if (IS_ENABLED(CONFIG_64BIT))
> - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
> + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
> else
> - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
> + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
> }
>
> static int can_stats_proc_show(struct seq_file *m, void *v)
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 139d9471ddcf..acfbae28d451 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -187,13 +187,13 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
>
> for (i = 0; i < count; i++) {
> err = can_rx_register(net, dev, filter[i].can_id,
> - filter[i].can_mask,
> + filter[i].can_mask, false,
> raw_rcv, sk, "raw", sk);
> if (err) {
> /* clean up successfully registered filters */
> while (--i >= 0)
> can_rx_unregister(net, dev, filter[i].can_id,
> - filter[i].can_mask,
> + filter[i].can_mask, false,
> raw_rcv, sk);
> break;
> }
> @@ -209,7 +209,7 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev,
>
> if (err_mask)
> err = can_rx_register(net, dev, 0, err_mask | CAN_ERR_FLAG,
> - raw_rcv, sk, "raw", sk);
> + false, raw_rcv, sk, "raw", sk);
>
> return err;
> }
> @@ -222,7 +222,7 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
>
> for (i = 0; i < count; i++)
> can_rx_unregister(net, dev, filter[i].can_id,
> - filter[i].can_mask, raw_rcv, sk);
> + filter[i].can_mask, false, raw_rcv, sk);
> }
>
> static inline void raw_disable_errfilter(struct net *net,
> @@ -233,7 +233,7 @@ static inline void raw_disable_errfilter(struct net *net,
> {
> if (err_mask)
> can_rx_unregister(net, dev, 0, err_mask | CAN_ERR_FLAG,
> - raw_rcv, sk);
> + false, raw_rcv, sk);
> }
>
> static inline void raw_disable_allfilters(struct net *net,
>
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-05 6:58 ` Oliver Hartkopp
@ 2021-05-05 8:37 ` Erik Flodin
2021-05-05 9:15 ` Oliver Hartkopp
0 siblings, 1 reply; 13+ messages in thread
From: Erik Flodin @ 2021-05-05 8:37 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
Hi,
On Wed, 5 May 2021 at 08:59, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> IMO putting linux-can@vger.kernel.org in CC would be sufficient in this
> stage of discussion.
OK, I've trimmed the receive list.
> On 04.05.21 22:35, Erik Flodin wrote:
> > Add a new flag to can_rx_register/unregister that, when set, requires
> > that the received sk_buff's owning socket matches the socket pointer
> > given when setting the filter. This makes it possible to set a filter
> > that matches all frames sent on a given socket and nothing else.
>
> This is a pretty simple requirement and you are touching almost every
> CAN network layer file.
>
> From this description I would suggest a two-liner to be added in raw.c
>
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 139d9471ddcf..8c6371a0c8a6 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -123,10 +123,14 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>
> /* check the received tx sock reference */
> if (!ro->recv_own_msgs && oskb->sk == sk)
> return;
>
> + /* check the received tx sock reference */
> + if (ro->recv_only_own_msgs && oskb->sk != sk)
> + return;
> +
> /* do not pass non-CAN2.0 frames to a legacy socket */
> if (!ro->fd_frames && oskb->len != CAN_MTU)
> return;
>
> /* eliminate multiple filter matches for the same skb */
>
>
> And then add CAN_RAW_RECV_ONLY_OWN_MSGS accordingly.
>
> Wouldn't that already match your requirements?
I guess that would require me to use two sockets: one for sending
(with RECV_ONLY_OWN_MSGS set) and one for receiving. The problem might
then be how to avoid the second socket seeing the frames sent from the
first, without disabling loopback completly?
// Erik
> >
> > Signed-off-by: Erik Flodin <erik@flodin.me>
> > ---
> > include/linux/can/core.h | 4 ++--
> > net/can/af_can.c | 50 ++++++++++++++++++++++------------------
> > net/can/af_can.h | 1 +
> > net/can/bcm.c | 9 +++++---
> > net/can/gw.c | 7 +++---
> > net/can/isotp.c | 8 +++----
> > net/can/j1939/main.c | 4 ++--
> > net/can/proc.c | 11 +++++----
> > net/can/raw.c | 10 ++++----
> > 9 files changed, 58 insertions(+), 46 deletions(-)
> >
> > diff --git a/include/linux/can/core.h b/include/linux/can/core.h
> > index 5fb8d0e3f9c1..7ee68128dc10 100644
> > --- a/include/linux/can/core.h
> > +++ b/include/linux/can/core.h
> > @@ -48,12 +48,12 @@ extern int can_proto_register(const struct can_proto *cp);
> > extern void can_proto_unregister(const struct can_proto *cp);
> >
> > int can_rx_register(struct net *net, struct net_device *dev,
> > - canid_t can_id, canid_t mask,
> > + canid_t can_id, canid_t mask, bool match_sk,
> > void (*func)(struct sk_buff *, void *),
> > void *data, char *ident, struct sock *sk);
> >
> > extern void can_rx_unregister(struct net *net, struct net_device *dev,
> > - canid_t can_id, canid_t mask,
> > + canid_t can_id, canid_t mask, bool match_sk,
> > void (*func)(struct sk_buff *, void *),
> > void *data);
> >
> > diff --git a/net/can/af_can.c b/net/can/af_can.c
> > index cce2af10eb3e..7b639c121653 100644
> > --- a/net/can/af_can.c
> > +++ b/net/can/af_can.c
> > @@ -414,6 +414,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
> > * @dev: pointer to netdevice (NULL => subscribe from 'all' CAN devices list)
> > * @can_id: CAN identifier (see description)
> > * @mask: CAN mask (see description)
> > + * @match_sk: match socket pointer on received sk_buff (see description)
> > * @func: callback function on filter match
> > * @data: returned parameter for callback function
> > * @ident: string for calling module identification
> > @@ -428,6 +429,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
> > * The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can
> > * filter for error message frames (CAN_ERR_FLAG bit set in mask).
> > *
> > + * If match_sk is true, the received sk_buff's owning socket must also match
> > + * the given socket pointer.
> > + *
> > * The provided pointer to the sk_buff is guaranteed to be valid as long as
> > * the callback function is running. The callback function must *not* free
> > * the given sk_buff while processing it's task. When the given sk_buff is
> > @@ -440,8 +444,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
> > * -ENODEV unknown device
> > */
> > int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
> > - canid_t mask, void (*func)(struct sk_buff *, void *),
> > - void *data, char *ident, struct sock *sk)
> > + canid_t mask, bool match_sk,
> > + void (*func)(struct sk_buff *, void *), void *data,
> > + char *ident, struct sock *sk)
> > {
> > struct receiver *rcv;
> > struct hlist_head *rcv_list;
> > @@ -468,6 +473,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
> >
> > rcv->can_id = can_id;
> > rcv->mask = mask;
> > + rcv->match_sk = match_sk;
> > rcv->matches = 0;
> > rcv->func = func;
> > rcv->data = data;
> > @@ -503,6 +509,7 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
> > * @dev: pointer to netdevice (NULL => unsubscribe from 'all' CAN devices list)
> > * @can_id: CAN identifier
> > * @mask: CAN mask
> > + * @match_sk: match socket pointer on received sk_buff
> > * @func: callback function on filter match
> > * @data: returned parameter for callback function
> > *
> > @@ -510,8 +517,8 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
> > * Removes subscription entry depending on given (subscription) values.
> > */
> > void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> > - canid_t mask, void (*func)(struct sk_buff *, void *),
> > - void *data)
> > + canid_t mask, bool match_sk,
> > + void (*func)(struct sk_buff *, void *), void *data)
> > {
> > struct receiver *rcv = NULL;
> > struct hlist_head *rcv_list;
> > @@ -535,7 +542,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> > */
> > hlist_for_each_entry_rcu(rcv, rcv_list, list) {
> > if (rcv->can_id == can_id && rcv->mask == mask &&
> > - rcv->func == func && rcv->data == data)
> > + rcv->match_sk == match_sk && rcv->func == func &&
> > + rcv->data == data)
> > break;
> > }
> >
> > @@ -546,8 +554,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> > * a warning here.
> > */
> > if (!rcv) {
> > - pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
> > - DNAME(dev), can_id, mask);
> > + pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X%s\n",
> > + DNAME(dev), can_id, mask, match_sk ? " (match sk)" : "");
> > goto out;
> > }
> >
> > @@ -569,10 +577,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> > }
> > EXPORT_SYMBOL(can_rx_unregister);
> >
> > -static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
> > +static inline int deliver(struct sk_buff *skb, struct receiver *rcv)
> > {
> > - rcv->func(skb, rcv->data);
> > - rcv->matches++;
> > + if (!rcv->match_sk || skb->sk == rcv->sk) {
> > + rcv->func(skb, rcv->data);
> > + rcv->matches++;
> > + return 1;
> > + }
> > + return 0;
> > }
> >
> > static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buff *skb)
> > @@ -589,8 +601,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
> > /* check for error message frame entries only */
> > hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ERR], list) {
> > if (can_id & rcv->mask) {
> > - deliver(skb, rcv);
> > - matches++;
> > + matches += deliver(skb, rcv);
> > }
> > }
> > return matches;
> > @@ -598,23 +609,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
> >
> > /* check for unfiltered entries */
> > hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ALL], list) {
> > - deliver(skb, rcv);
> > - matches++;
> > + matches += deliver(skb, rcv);
> > }
> >
> > /* check for can_id/mask entries */
> > hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_FIL], list) {
> > if ((can_id & rcv->mask) == rcv->can_id) {
> > - deliver(skb, rcv);
> > - matches++;
> > + matches += deliver(skb, rcv);
> > }
> > }
> >
> > /* check for inverted can_id/mask entries */
> > hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_INV], list) {
> > if ((can_id & rcv->mask) != rcv->can_id) {
> > - deliver(skb, rcv);
> > - matches++;
> > + matches += deliver(skb, rcv);
> > }
> > }
> >
> > @@ -625,15 +633,13 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
> > if (can_id & CAN_EFF_FLAG) {
> > hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_eff[effhash(can_id)], list) {
> > if (rcv->can_id == can_id) {
> > - deliver(skb, rcv);
> > - matches++;
> > + matches += deliver(skb, rcv);
> > }
> > }
> > } else {
> > can_id &= CAN_SFF_MASK;
> > hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_sff[can_id], list) {
> > - deliver(skb, rcv);
> > - matches++;
> > + matches += deliver(skb, rcv);
> > }
> > }
> >
> > diff --git a/net/can/af_can.h b/net/can/af_can.h
> > index 7c2d9161e224..ea98b10d93e7 100644
> > --- a/net/can/af_can.h
> > +++ b/net/can/af_can.h
> > @@ -52,6 +52,7 @@ struct receiver {
> > struct hlist_node list;
> > canid_t can_id;
> > canid_t mask;
> > + bool match_sk;
> > unsigned long matches;
> > void (*func)(struct sk_buff *skb, void *data);
> > void *data;
> > diff --git a/net/can/bcm.c b/net/can/bcm.c
> > index 909b9e684e04..d89dd82c2178 100644
> > --- a/net/can/bcm.c
> > +++ b/net/can/bcm.c
> > @@ -729,7 +729,8 @@ static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
> > {
> > if (op->rx_reg_dev == dev) {
> > can_rx_unregister(dev_net(dev), dev, op->can_id,
> > - REGMASK(op->can_id), bcm_rx_handler, op);
> > + REGMASK(op->can_id), false, bcm_rx_handler,
> > + op);
> >
> > /* mark as removed subscription */
> > op->rx_reg_dev = NULL;
> > @@ -775,6 +776,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
> > can_rx_unregister(sock_net(op->sk), NULL,
> > op->can_id,
> > REGMASK(op->can_id),
> > + false,
> > bcm_rx_handler, op);
> >
> > list_del(&op->list);
> > @@ -1193,6 +1195,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> > err = can_rx_register(sock_net(sk), dev,
> > op->can_id,
> > REGMASK(op->can_id),
> > + false,
> > bcm_rx_handler, op,
> > "bcm", sk);
> >
> > @@ -1202,7 +1205,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> >
> > } else
> > err = can_rx_register(sock_net(sk), NULL, op->can_id,
> > - REGMASK(op->can_id),
> > + REGMASK(op->can_id), false,
> > bcm_rx_handler, op, "bcm", sk);
> > if (err) {
> > /* this bcm rx op is broken -> remove it */
> > @@ -1500,7 +1503,7 @@ static int bcm_release(struct socket *sock)
> > }
> > } else
> > can_rx_unregister(net, NULL, op->can_id,
> > - REGMASK(op->can_id),
> > + REGMASK(op->can_id), false,
> > bcm_rx_handler, op);
> >
> > bcm_remove_op(op);
> > diff --git a/net/can/gw.c b/net/can/gw.c
> > index ba4124805602..5dbc7b85e0fc 100644
> > --- a/net/can/gw.c
> > +++ b/net/can/gw.c
> > @@ -567,14 +567,15 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
> > static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
> > {
> > return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id,
> > - gwj->ccgw.filter.can_mask, can_can_gw_rcv,
> > - gwj, "gw", NULL);
> > + gwj->ccgw.filter.can_mask, false,
> > + can_can_gw_rcv, gwj, "gw", NULL);
> > }
> >
> > static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
> > {
> > can_rx_unregister(net, gwj->src.dev, gwj->ccgw.filter.can_id,
> > - gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
> > + gwj->ccgw.filter.can_mask, false, can_can_gw_rcv,
> > + gwj);
> > }
> >
> > static int cgw_notifier(struct notifier_block *nb,
> > diff --git a/net/can/isotp.c b/net/can/isotp.c
> > index 9f94ad3caee9..44d943bbe0b1 100644
> > --- a/net/can/isotp.c
> > +++ b/net/can/isotp.c
> > @@ -1029,7 +1029,7 @@ static int isotp_release(struct socket *sock)
> > if (dev) {
> > can_rx_unregister(net, dev, so->rxid,
> > SINGLE_MASK(so->rxid),
> > - isotp_rcv, sk);
> > + false, isotp_rcv, sk);
> > dev_put(dev);
> > }
> > }
> > @@ -1111,7 +1111,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> > if (do_rx_reg)
> > can_rx_register(net, dev, addr->can_addr.tp.rx_id,
> > SINGLE_MASK(addr->can_addr.tp.rx_id),
> > - isotp_rcv, sk, "isotp", sk);
> > + false, isotp_rcv, sk, "isotp", sk);
> >
> > dev_put(dev);
> >
> > @@ -1122,7 +1122,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> > if (dev) {
> > can_rx_unregister(net, dev, so->rxid,
> > SINGLE_MASK(so->rxid),
> > - isotp_rcv, sk);
> > + false, isotp_rcv, sk);
> > dev_put(dev);
> > }
> > }
> > @@ -1323,7 +1323,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
> > if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST)))
> > can_rx_unregister(dev_net(dev), dev, so->rxid,
> > SINGLE_MASK(so->rxid),
> > - isotp_rcv, sk);
> > + false, isotp_rcv, sk);
> >
> > so->ifindex = 0;
> > so->bound = 0;
> > diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> > index da3a7a7bcff2..466a20c76fb6 100644
> > --- a/net/can/j1939/main.c
> > +++ b/net/can/j1939/main.c
> > @@ -177,7 +177,7 @@ static int j1939_can_rx_register(struct j1939_priv *priv)
> >
> > j1939_priv_get(priv);
> > ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> > - j1939_can_recv, priv, "j1939", NULL);
> > + false, j1939_can_recv, priv, "j1939", NULL);
> > if (ret < 0) {
> > j1939_priv_put(priv);
> > return ret;
> > @@ -191,7 +191,7 @@ static void j1939_can_rx_unregister(struct j1939_priv *priv)
> > struct net_device *ndev = priv->ndev;
> >
> > can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> > - j1939_can_recv, priv);
> > + false, j1939_can_recv, priv);
> >
> > j1939_priv_put(priv);
> > }
> > diff --git a/net/can/proc.c b/net/can/proc.c
> > index d1fe49e6f16d..d312077832a6 100644
> > --- a/net/can/proc.c
> > +++ b/net/can/proc.c
> > @@ -191,11 +191,12 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
> >
> > hlist_for_each_entry_rcu(r, rx_list, list) {
> > char *fmt = (r->can_id & CAN_EFF_FLAG)?
> > - " %-5s %08x %08x %pK %pK %8ld %s\n" :
> > - " %-5s %03x %08x %pK %pK %8ld %s\n";
> > + " %-5s %08x %08x %c %pK %pK %8ld %s\n" :
> > + " %-5s %03x %08x %c %pK %pK %8ld %s\n";
> >
> > seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
> > - r->func, r->data, r->matches, r->ident);
> > + r->match_sk ? '*' : ' ', r->func, r->data,
> > + r->matches, r->ident);
> > }
> > }
> >
> > @@ -206,9 +207,9 @@ static void can_print_recv_banner(struct seq_file *m)
> > * ....... 0 tp20
> > */
> > if (IS_ENABLED(CONFIG_64BIT))
> > - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
> > + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
> > else
> > - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
> > + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
> > }
> >
> > static int can_stats_proc_show(struct seq_file *m, void *v)
> > diff --git a/net/can/raw.c b/net/can/raw.c
> > index 139d9471ddcf..acfbae28d451 100644
> > --- a/net/can/raw.c
> > +++ b/net/can/raw.c
> > @@ -187,13 +187,13 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
> >
> > for (i = 0; i < count; i++) {
> > err = can_rx_register(net, dev, filter[i].can_id,
> > - filter[i].can_mask,
> > + filter[i].can_mask, false,
> > raw_rcv, sk, "raw", sk);
> > if (err) {
> > /* clean up successfully registered filters */
> > while (--i >= 0)
> > can_rx_unregister(net, dev, filter[i].can_id,
> > - filter[i].can_mask,
> > + filter[i].can_mask, false,
> > raw_rcv, sk);
> > break;
> > }
> > @@ -209,7 +209,7 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev,
> >
> > if (err_mask)
> > err = can_rx_register(net, dev, 0, err_mask | CAN_ERR_FLAG,
> > - raw_rcv, sk, "raw", sk);
> > + false, raw_rcv, sk, "raw", sk);
> >
> > return err;
> > }
> > @@ -222,7 +222,7 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
> >
> > for (i = 0; i < count; i++)
> > can_rx_unregister(net, dev, filter[i].can_id,
> > - filter[i].can_mask, raw_rcv, sk);
> > + filter[i].can_mask, false, raw_rcv, sk);
> > }
> >
> > static inline void raw_disable_errfilter(struct net *net,
> > @@ -233,7 +233,7 @@ static inline void raw_disable_errfilter(struct net *net,
> > {
> > if (err_mask)
> > can_rx_unregister(net, dev, 0, err_mask | CAN_ERR_FLAG,
> > - raw_rcv, sk);
> > + false, raw_rcv, sk);
> > }
> >
> > static inline void raw_disable_allfilters(struct net *net,
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-05 8:37 ` Erik Flodin
@ 2021-05-05 9:15 ` Oliver Hartkopp
2021-05-05 18:54 ` Erik Flodin
0 siblings, 1 reply; 13+ messages in thread
From: Oliver Hartkopp @ 2021-05-05 9:15 UTC (permalink / raw)
To: Erik Flodin; +Cc: linux-can
On 05.05.21 10:37, Erik Flodin wrote:
>>> Add a new flag to can_rx_register/unregister that, when set, requires
>>> that the received sk_buff's owning socket matches the socket pointer
>>> given when setting the filter. This makes it possible to set a filter
>>> that matches all frames sent on a given socket and nothing else.
>>
(..)
>>
>> Wouldn't that already match your requirements?
>
> I guess that would require me to use two sockets: one for sending
> (with RECV_ONLY_OWN_MSGS set) and one for receiving. The problem might
> then be how to avoid the second socket seeing the frames sent from the
> first, without disabling loopback completly?
This is indeed a question of the application layout.
From what I understood your main requirement is to double check the
outgoing traffic whether is has been sent. To me it makes sense to use a
separate socket for sending that triggers the special handling of send
acknowledges when receiving frames (on that sending socket).
And then use a separate socket to receive frames needed by the application.
In both cases (even with your one-socket solution) you need to implement
some more or less complex dispatching in user space when receiving frames.
IMO it would make sense to use a sending socket (including the TX ACK
handling) and a separate receiving socket, where you should set the
appropriate filters needed for your application anyway.
Best regards,
Oliver
>
>>>
>>> Signed-off-by: Erik Flodin <erik@flodin.me>
>>> ---
>>> include/linux/can/core.h | 4 ++--
>>> net/can/af_can.c | 50 ++++++++++++++++++++++------------------
>>> net/can/af_can.h | 1 +
>>> net/can/bcm.c | 9 +++++---
>>> net/can/gw.c | 7 +++---
>>> net/can/isotp.c | 8 +++----
>>> net/can/j1939/main.c | 4 ++--
>>> net/can/proc.c | 11 +++++----
>>> net/can/raw.c | 10 ++++----
>>> 9 files changed, 58 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/include/linux/can/core.h b/include/linux/can/core.h
>>> index 5fb8d0e3f9c1..7ee68128dc10 100644
>>> --- a/include/linux/can/core.h
>>> +++ b/include/linux/can/core.h
>>> @@ -48,12 +48,12 @@ extern int can_proto_register(const struct can_proto *cp);
>>> extern void can_proto_unregister(const struct can_proto *cp);
>>>
>>> int can_rx_register(struct net *net, struct net_device *dev,
>>> - canid_t can_id, canid_t mask,
>>> + canid_t can_id, canid_t mask, bool match_sk,
>>> void (*func)(struct sk_buff *, void *),
>>> void *data, char *ident, struct sock *sk);
>>>
>>> extern void can_rx_unregister(struct net *net, struct net_device *dev,
>>> - canid_t can_id, canid_t mask,
>>> + canid_t can_id, canid_t mask, bool match_sk,
>>> void (*func)(struct sk_buff *, void *),
>>> void *data);
>>>
>>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>>> index cce2af10eb3e..7b639c121653 100644
>>> --- a/net/can/af_can.c
>>> +++ b/net/can/af_can.c
>>> @@ -414,6 +414,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
>>> * @dev: pointer to netdevice (NULL => subscribe from 'all' CAN devices list)
>>> * @can_id: CAN identifier (see description)
>>> * @mask: CAN mask (see description)
>>> + * @match_sk: match socket pointer on received sk_buff (see description)
>>> * @func: callback function on filter match
>>> * @data: returned parameter for callback function
>>> * @ident: string for calling module identification
>>> @@ -428,6 +429,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
>>> * The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can
>>> * filter for error message frames (CAN_ERR_FLAG bit set in mask).
>>> *
>>> + * If match_sk is true, the received sk_buff's owning socket must also match
>>> + * the given socket pointer.
>>> + *
>>> * The provided pointer to the sk_buff is guaranteed to be valid as long as
>>> * the callback function is running. The callback function must *not* free
>>> * the given sk_buff while processing it's task. When the given sk_buff is
>>> @@ -440,8 +444,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
>>> * -ENODEV unknown device
>>> */
>>> int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>>> - canid_t mask, void (*func)(struct sk_buff *, void *),
>>> - void *data, char *ident, struct sock *sk)
>>> + canid_t mask, bool match_sk,
>>> + void (*func)(struct sk_buff *, void *), void *data,
>>> + char *ident, struct sock *sk)
>>> {
>>> struct receiver *rcv;
>>> struct hlist_head *rcv_list;
>>> @@ -468,6 +473,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>>>
>>> rcv->can_id = can_id;
>>> rcv->mask = mask;
>>> + rcv->match_sk = match_sk;
>>> rcv->matches = 0;
>>> rcv->func = func;
>>> rcv->data = data;
>>> @@ -503,6 +509,7 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
>>> * @dev: pointer to netdevice (NULL => unsubscribe from 'all' CAN devices list)
>>> * @can_id: CAN identifier
>>> * @mask: CAN mask
>>> + * @match_sk: match socket pointer on received sk_buff
>>> * @func: callback function on filter match
>>> * @data: returned parameter for callback function
>>> *
>>> @@ -510,8 +517,8 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
>>> * Removes subscription entry depending on given (subscription) values.
>>> */
>>> void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>>> - canid_t mask, void (*func)(struct sk_buff *, void *),
>>> - void *data)
>>> + canid_t mask, bool match_sk,
>>> + void (*func)(struct sk_buff *, void *), void *data)
>>> {
>>> struct receiver *rcv = NULL;
>>> struct hlist_head *rcv_list;
>>> @@ -535,7 +542,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>>> */
>>> hlist_for_each_entry_rcu(rcv, rcv_list, list) {
>>> if (rcv->can_id == can_id && rcv->mask == mask &&
>>> - rcv->func == func && rcv->data == data)
>>> + rcv->match_sk == match_sk && rcv->func == func &&
>>> + rcv->data == data)
>>> break;
>>> }
>>>
>>> @@ -546,8 +554,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>>> * a warning here.
>>> */
>>> if (!rcv) {
>>> - pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
>>> - DNAME(dev), can_id, mask);
>>> + pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X%s\n",
>>> + DNAME(dev), can_id, mask, match_sk ? " (match sk)" : "");
>>> goto out;
>>> }
>>>
>>> @@ -569,10 +577,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>>> }
>>> EXPORT_SYMBOL(can_rx_unregister);
>>>
>>> -static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
>>> +static inline int deliver(struct sk_buff *skb, struct receiver *rcv)
>>> {
>>> - rcv->func(skb, rcv->data);
>>> - rcv->matches++;
>>> + if (!rcv->match_sk || skb->sk == rcv->sk) {
>>> + rcv->func(skb, rcv->data);
>>> + rcv->matches++;
>>> + return 1;
>>> + }
>>> + return 0;
>>> }
>>>
>>> static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buff *skb)
>>> @@ -589,8 +601,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
>>> /* check for error message frame entries only */
>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ERR], list) {
>>> if (can_id & rcv->mask) {
>>> - deliver(skb, rcv);
>>> - matches++;
>>> + matches += deliver(skb, rcv);
>>> }
>>> }
>>> return matches;
>>> @@ -598,23 +609,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
>>>
>>> /* check for unfiltered entries */
>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ALL], list) {
>>> - deliver(skb, rcv);
>>> - matches++;
>>> + matches += deliver(skb, rcv);
>>> }
>>>
>>> /* check for can_id/mask entries */
>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_FIL], list) {
>>> if ((can_id & rcv->mask) == rcv->can_id) {
>>> - deliver(skb, rcv);
>>> - matches++;
>>> + matches += deliver(skb, rcv);
>>> }
>>> }
>>>
>>> /* check for inverted can_id/mask entries */
>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_INV], list) {
>>> if ((can_id & rcv->mask) != rcv->can_id) {
>>> - deliver(skb, rcv);
>>> - matches++;
>>> + matches += deliver(skb, rcv);
>>> }
>>> }
>>>
>>> @@ -625,15 +633,13 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
>>> if (can_id & CAN_EFF_FLAG) {
>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_eff[effhash(can_id)], list) {
>>> if (rcv->can_id == can_id) {
>>> - deliver(skb, rcv);
>>> - matches++;
>>> + matches += deliver(skb, rcv);
>>> }
>>> }
>>> } else {
>>> can_id &= CAN_SFF_MASK;
>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_sff[can_id], list) {
>>> - deliver(skb, rcv);
>>> - matches++;
>>> + matches += deliver(skb, rcv);
>>> }
>>> }
>>>
>>> diff --git a/net/can/af_can.h b/net/can/af_can.h
>>> index 7c2d9161e224..ea98b10d93e7 100644
>>> --- a/net/can/af_can.h
>>> +++ b/net/can/af_can.h
>>> @@ -52,6 +52,7 @@ struct receiver {
>>> struct hlist_node list;
>>> canid_t can_id;
>>> canid_t mask;
>>> + bool match_sk;
>>> unsigned long matches;
>>> void (*func)(struct sk_buff *skb, void *data);
>>> void *data;
>>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>>> index 909b9e684e04..d89dd82c2178 100644
>>> --- a/net/can/bcm.c
>>> +++ b/net/can/bcm.c
>>> @@ -729,7 +729,8 @@ static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
>>> {
>>> if (op->rx_reg_dev == dev) {
>>> can_rx_unregister(dev_net(dev), dev, op->can_id,
>>> - REGMASK(op->can_id), bcm_rx_handler, op);
>>> + REGMASK(op->can_id), false, bcm_rx_handler,
>>> + op);
>>>
>>> /* mark as removed subscription */
>>> op->rx_reg_dev = NULL;
>>> @@ -775,6 +776,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
>>> can_rx_unregister(sock_net(op->sk), NULL,
>>> op->can_id,
>>> REGMASK(op->can_id),
>>> + false,
>>> bcm_rx_handler, op);
>>>
>>> list_del(&op->list);
>>> @@ -1193,6 +1195,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>>> err = can_rx_register(sock_net(sk), dev,
>>> op->can_id,
>>> REGMASK(op->can_id),
>>> + false,
>>> bcm_rx_handler, op,
>>> "bcm", sk);
>>>
>>> @@ -1202,7 +1205,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>>>
>>> } else
>>> err = can_rx_register(sock_net(sk), NULL, op->can_id,
>>> - REGMASK(op->can_id),
>>> + REGMASK(op->can_id), false,
>>> bcm_rx_handler, op, "bcm", sk);
>>> if (err) {
>>> /* this bcm rx op is broken -> remove it */
>>> @@ -1500,7 +1503,7 @@ static int bcm_release(struct socket *sock)
>>> }
>>> } else
>>> can_rx_unregister(net, NULL, op->can_id,
>>> - REGMASK(op->can_id),
>>> + REGMASK(op->can_id), false,
>>> bcm_rx_handler, op);
>>>
>>> bcm_remove_op(op);
>>> diff --git a/net/can/gw.c b/net/can/gw.c
>>> index ba4124805602..5dbc7b85e0fc 100644
>>> --- a/net/can/gw.c
>>> +++ b/net/can/gw.c
>>> @@ -567,14 +567,15 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>>> static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
>>> {
>>> return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id,
>>> - gwj->ccgw.filter.can_mask, can_can_gw_rcv,
>>> - gwj, "gw", NULL);
>>> + gwj->ccgw.filter.can_mask, false,
>>> + can_can_gw_rcv, gwj, "gw", NULL);
>>> }
>>>
>>> static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
>>> {
>>> can_rx_unregister(net, gwj->src.dev, gwj->ccgw.filter.can_id,
>>> - gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
>>> + gwj->ccgw.filter.can_mask, false, can_can_gw_rcv,
>>> + gwj);
>>> }
>>>
>>> static int cgw_notifier(struct notifier_block *nb,
>>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>>> index 9f94ad3caee9..44d943bbe0b1 100644
>>> --- a/net/can/isotp.c
>>> +++ b/net/can/isotp.c
>>> @@ -1029,7 +1029,7 @@ static int isotp_release(struct socket *sock)
>>> if (dev) {
>>> can_rx_unregister(net, dev, so->rxid,
>>> SINGLE_MASK(so->rxid),
>>> - isotp_rcv, sk);
>>> + false, isotp_rcv, sk);
>>> dev_put(dev);
>>> }
>>> }
>>> @@ -1111,7 +1111,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>>> if (do_rx_reg)
>>> can_rx_register(net, dev, addr->can_addr.tp.rx_id,
>>> SINGLE_MASK(addr->can_addr.tp.rx_id),
>>> - isotp_rcv, sk, "isotp", sk);
>>> + false, isotp_rcv, sk, "isotp", sk);
>>>
>>> dev_put(dev);
>>>
>>> @@ -1122,7 +1122,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>>> if (dev) {
>>> can_rx_unregister(net, dev, so->rxid,
>>> SINGLE_MASK(so->rxid),
>>> - isotp_rcv, sk);
>>> + false, isotp_rcv, sk);
>>> dev_put(dev);
>>> }
>>> }
>>> @@ -1323,7 +1323,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
>>> if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST)))
>>> can_rx_unregister(dev_net(dev), dev, so->rxid,
>>> SINGLE_MASK(so->rxid),
>>> - isotp_rcv, sk);
>>> + false, isotp_rcv, sk);
>>>
>>> so->ifindex = 0;
>>> so->bound = 0;
>>> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
>>> index da3a7a7bcff2..466a20c76fb6 100644
>>> --- a/net/can/j1939/main.c
>>> +++ b/net/can/j1939/main.c
>>> @@ -177,7 +177,7 @@ static int j1939_can_rx_register(struct j1939_priv *priv)
>>>
>>> j1939_priv_get(priv);
>>> ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
>>> - j1939_can_recv, priv, "j1939", NULL);
>>> + false, j1939_can_recv, priv, "j1939", NULL);
>>> if (ret < 0) {
>>> j1939_priv_put(priv);
>>> return ret;
>>> @@ -191,7 +191,7 @@ static void j1939_can_rx_unregister(struct j1939_priv *priv)
>>> struct net_device *ndev = priv->ndev;
>>>
>>> can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
>>> - j1939_can_recv, priv);
>>> + false, j1939_can_recv, priv);
>>>
>>> j1939_priv_put(priv);
>>> }
>>> diff --git a/net/can/proc.c b/net/can/proc.c
>>> index d1fe49e6f16d..d312077832a6 100644
>>> --- a/net/can/proc.c
>>> +++ b/net/can/proc.c
>>> @@ -191,11 +191,12 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
>>>
>>> hlist_for_each_entry_rcu(r, rx_list, list) {
>>> char *fmt = (r->can_id & CAN_EFF_FLAG)?
>>> - " %-5s %08x %08x %pK %pK %8ld %s\n" :
>>> - " %-5s %03x %08x %pK %pK %8ld %s\n";
>>> + " %-5s %08x %08x %c %pK %pK %8ld %s\n" :
>>> + " %-5s %03x %08x %c %pK %pK %8ld %s\n";
>>>
>>> seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
>>> - r->func, r->data, r->matches, r->ident);
>>> + r->match_sk ? '*' : ' ', r->func, r->data,
>>> + r->matches, r->ident);
>>> }
>>> }
>>>
>>> @@ -206,9 +207,9 @@ static void can_print_recv_banner(struct seq_file *m)
>>> * ....... 0 tp20
>>> */
>>> if (IS_ENABLED(CONFIG_64BIT))
>>> - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
>>> + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
>>> else
>>> - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
>>> + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
>>> }
>>>
>>> static int can_stats_proc_show(struct seq_file *m, void *v)
>>> diff --git a/net/can/raw.c b/net/can/raw.c
>>> index 139d9471ddcf..acfbae28d451 100644
>>> --- a/net/can/raw.c
>>> +++ b/net/can/raw.c
>>> @@ -187,13 +187,13 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
>>>
>>> for (i = 0; i < count; i++) {
>>> err = can_rx_register(net, dev, filter[i].can_id,
>>> - filter[i].can_mask,
>>> + filter[i].can_mask, false,
>>> raw_rcv, sk, "raw", sk);
>>> if (err) {
>>> /* clean up successfully registered filters */
>>> while (--i >= 0)
>>> can_rx_unregister(net, dev, filter[i].can_id,
>>> - filter[i].can_mask,
>>> + filter[i].can_mask, false,
>>> raw_rcv, sk);
>>> break;
>>> }
>>> @@ -209,7 +209,7 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev,
>>>
>>> if (err_mask)
>>> err = can_rx_register(net, dev, 0, err_mask | CAN_ERR_FLAG,
>>> - raw_rcv, sk, "raw", sk);
>>> + false, raw_rcv, sk, "raw", sk);
>>>
>>> return err;
>>> }
>>> @@ -222,7 +222,7 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
>>>
>>> for (i = 0; i < count; i++)
>>> can_rx_unregister(net, dev, filter[i].can_id,
>>> - filter[i].can_mask, raw_rcv, sk);
>>> + filter[i].can_mask, false, raw_rcv, sk);
>>> }
>>>
>>> static inline void raw_disable_errfilter(struct net *net,
>>> @@ -233,7 +233,7 @@ static inline void raw_disable_errfilter(struct net *net,
>>> {
>>> if (err_mask)
>>> can_rx_unregister(net, dev, 0, err_mask | CAN_ERR_FLAG,
>>> - raw_rcv, sk);
>>> + false, raw_rcv, sk);
>>> }
>>>
>>> static inline void raw_disable_allfilters(struct net *net,
>>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-05 9:15 ` Oliver Hartkopp
@ 2021-05-05 18:54 ` Erik Flodin
2021-05-06 5:26 ` Oliver Hartkopp
0 siblings, 1 reply; 13+ messages in thread
From: Erik Flodin @ 2021-05-05 18:54 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
On Wed, 5 May 2021 at 11:15, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 05.05.21 10:37, Erik Flodin wrote:
>
> >>> Add a new flag to can_rx_register/unregister that, when set, requires
> >>> that the received sk_buff's owning socket matches the socket pointer
> >>> given when setting the filter. This makes it possible to set a filter
> >>> that matches all frames sent on a given socket and nothing else.
> >>
>
> (..)
>
> >>
> >> Wouldn't that already match your requirements?
> >
> > I guess that would require me to use two sockets: one for sending
> > (with RECV_ONLY_OWN_MSGS set) and one for receiving. The problem might
> > then be how to avoid the second socket seeing the frames sent from the
> > first, without disabling loopback completly?
>
> This is indeed a question of the application layout.
>
> From what I understood your main requirement is to double check the
> outgoing traffic whether is has been sent.
What I would like to have is:
1. Be notified when a frame has been sent. I send (from user space) a
single frame and wait until I get confirmation before sending the next
to give other nodes on the bus a slot to start sending, even if their
frames have ID with lower priority.
2. I don't want to receive my own frames, except to support the use
case above. But then the kernel tells me that it's mine and I can
easily ignore it.
3. Both be able to run with receive filters set (when that makes
sense) or use the default catch-all filter in cases where I want to
receive everything and don't want to enumerate all possible CAN IDs.
4. Be able to use cansend and candump on the same host as above.
> To me it makes sense to use a
> separate socket for sending that triggers the special handling of send
> acknowledges when receiving frames (on that sending socket).
>
> And then use a separate socket to receive frames needed by the application.
To be able to use two sockets I would have to perform filtering in
user space as well in order to filter out frames sent on one socket
and received on the other. But for how long should I remember the sent
frame to be able to filter it out? It might not come at all (in the
case where there are filters set) and setting a time after which it is
forgotten, even if synchronized with the confirmation on the sending
socket, just doesn't feel rock solid.
> In both cases (even with your one-socket solution) you need to implement
> some more or less complex dispatching in user space when receiving frames.
What I do now is to keep the last sent frame around and when I get a
frame with MSG_CONFIRM set I can check that it's the one I sent,
ignore it and continue sending the next frame (if any).
// Erik
> >>>
> >>> Signed-off-by: Erik Flodin <erik@flodin.me>
> >>> ---
> >>> include/linux/can/core.h | 4 ++--
> >>> net/can/af_can.c | 50 ++++++++++++++++++++++------------------
> >>> net/can/af_can.h | 1 +
> >>> net/can/bcm.c | 9 +++++---
> >>> net/can/gw.c | 7 +++---
> >>> net/can/isotp.c | 8 +++----
> >>> net/can/j1939/main.c | 4 ++--
> >>> net/can/proc.c | 11 +++++----
> >>> net/can/raw.c | 10 ++++----
> >>> 9 files changed, 58 insertions(+), 46 deletions(-)
> >>>
> >>> diff --git a/include/linux/can/core.h b/include/linux/can/core.h
> >>> index 5fb8d0e3f9c1..7ee68128dc10 100644
> >>> --- a/include/linux/can/core.h
> >>> +++ b/include/linux/can/core.h
> >>> @@ -48,12 +48,12 @@ extern int can_proto_register(const struct can_proto *cp);
> >>> extern void can_proto_unregister(const struct can_proto *cp);
> >>>
> >>> int can_rx_register(struct net *net, struct net_device *dev,
> >>> - canid_t can_id, canid_t mask,
> >>> + canid_t can_id, canid_t mask, bool match_sk,
> >>> void (*func)(struct sk_buff *, void *),
> >>> void *data, char *ident, struct sock *sk);
> >>>
> >>> extern void can_rx_unregister(struct net *net, struct net_device *dev,
> >>> - canid_t can_id, canid_t mask,
> >>> + canid_t can_id, canid_t mask, bool match_sk,
> >>> void (*func)(struct sk_buff *, void *),
> >>> void *data);
> >>>
> >>> diff --git a/net/can/af_can.c b/net/can/af_can.c
> >>> index cce2af10eb3e..7b639c121653 100644
> >>> --- a/net/can/af_can.c
> >>> +++ b/net/can/af_can.c
> >>> @@ -414,6 +414,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
> >>> * @dev: pointer to netdevice (NULL => subscribe from 'all' CAN devices list)
> >>> * @can_id: CAN identifier (see description)
> >>> * @mask: CAN mask (see description)
> >>> + * @match_sk: match socket pointer on received sk_buff (see description)
> >>> * @func: callback function on filter match
> >>> * @data: returned parameter for callback function
> >>> * @ident: string for calling module identification
> >>> @@ -428,6 +429,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
> >>> * The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can
> >>> * filter for error message frames (CAN_ERR_FLAG bit set in mask).
> >>> *
> >>> + * If match_sk is true, the received sk_buff's owning socket must also match
> >>> + * the given socket pointer.
> >>> + *
> >>> * The provided pointer to the sk_buff is guaranteed to be valid as long as
> >>> * the callback function is running. The callback function must *not* free
> >>> * the given sk_buff while processing it's task. When the given sk_buff is
> >>> @@ -440,8 +444,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
> >>> * -ENODEV unknown device
> >>> */
> >>> int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
> >>> - canid_t mask, void (*func)(struct sk_buff *, void *),
> >>> - void *data, char *ident, struct sock *sk)
> >>> + canid_t mask, bool match_sk,
> >>> + void (*func)(struct sk_buff *, void *), void *data,
> >>> + char *ident, struct sock *sk)
> >>> {
> >>> struct receiver *rcv;
> >>> struct hlist_head *rcv_list;
> >>> @@ -468,6 +473,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
> >>>
> >>> rcv->can_id = can_id;
> >>> rcv->mask = mask;
> >>> + rcv->match_sk = match_sk;
> >>> rcv->matches = 0;
> >>> rcv->func = func;
> >>> rcv->data = data;
> >>> @@ -503,6 +509,7 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
> >>> * @dev: pointer to netdevice (NULL => unsubscribe from 'all' CAN devices list)
> >>> * @can_id: CAN identifier
> >>> * @mask: CAN mask
> >>> + * @match_sk: match socket pointer on received sk_buff
> >>> * @func: callback function on filter match
> >>> * @data: returned parameter for callback function
> >>> *
> >>> @@ -510,8 +517,8 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
> >>> * Removes subscription entry depending on given (subscription) values.
> >>> */
> >>> void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> >>> - canid_t mask, void (*func)(struct sk_buff *, void *),
> >>> - void *data)
> >>> + canid_t mask, bool match_sk,
> >>> + void (*func)(struct sk_buff *, void *), void *data)
> >>> {
> >>> struct receiver *rcv = NULL;
> >>> struct hlist_head *rcv_list;
> >>> @@ -535,7 +542,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> >>> */
> >>> hlist_for_each_entry_rcu(rcv, rcv_list, list) {
> >>> if (rcv->can_id == can_id && rcv->mask == mask &&
> >>> - rcv->func == func && rcv->data == data)
> >>> + rcv->match_sk == match_sk && rcv->func == func &&
> >>> + rcv->data == data)
> >>> break;
> >>> }
> >>>
> >>> @@ -546,8 +554,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> >>> * a warning here.
> >>> */
> >>> if (!rcv) {
> >>> - pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
> >>> - DNAME(dev), can_id, mask);
> >>> + pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X%s\n",
> >>> + DNAME(dev), can_id, mask, match_sk ? " (match sk)" : "");
> >>> goto out;
> >>> }
> >>>
> >>> @@ -569,10 +577,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
> >>> }
> >>> EXPORT_SYMBOL(can_rx_unregister);
> >>>
> >>> -static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
> >>> +static inline int deliver(struct sk_buff *skb, struct receiver *rcv)
> >>> {
> >>> - rcv->func(skb, rcv->data);
> >>> - rcv->matches++;
> >>> + if (!rcv->match_sk || skb->sk == rcv->sk) {
> >>> + rcv->func(skb, rcv->data);
> >>> + rcv->matches++;
> >>> + return 1;
> >>> + }
> >>> + return 0;
> >>> }
> >>>
> >>> static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buff *skb)
> >>> @@ -589,8 +601,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
> >>> /* check for error message frame entries only */
> >>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ERR], list) {
> >>> if (can_id & rcv->mask) {
> >>> - deliver(skb, rcv);
> >>> - matches++;
> >>> + matches += deliver(skb, rcv);
> >>> }
> >>> }
> >>> return matches;
> >>> @@ -598,23 +609,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
> >>>
> >>> /* check for unfiltered entries */
> >>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ALL], list) {
> >>> - deliver(skb, rcv);
> >>> - matches++;
> >>> + matches += deliver(skb, rcv);
> >>> }
> >>>
> >>> /* check for can_id/mask entries */
> >>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_FIL], list) {
> >>> if ((can_id & rcv->mask) == rcv->can_id) {
> >>> - deliver(skb, rcv);
> >>> - matches++;
> >>> + matches += deliver(skb, rcv);
> >>> }
> >>> }
> >>>
> >>> /* check for inverted can_id/mask entries */
> >>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_INV], list) {
> >>> if ((can_id & rcv->mask) != rcv->can_id) {
> >>> - deliver(skb, rcv);
> >>> - matches++;
> >>> + matches += deliver(skb, rcv);
> >>> }
> >>> }
> >>>
> >>> @@ -625,15 +633,13 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
> >>> if (can_id & CAN_EFF_FLAG) {
> >>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_eff[effhash(can_id)], list) {
> >>> if (rcv->can_id == can_id) {
> >>> - deliver(skb, rcv);
> >>> - matches++;
> >>> + matches += deliver(skb, rcv);
> >>> }
> >>> }
> >>> } else {
> >>> can_id &= CAN_SFF_MASK;
> >>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_sff[can_id], list) {
> >>> - deliver(skb, rcv);
> >>> - matches++;
> >>> + matches += deliver(skb, rcv);
> >>> }
> >>> }
> >>>
> >>> diff --git a/net/can/af_can.h b/net/can/af_can.h
> >>> index 7c2d9161e224..ea98b10d93e7 100644
> >>> --- a/net/can/af_can.h
> >>> +++ b/net/can/af_can.h
> >>> @@ -52,6 +52,7 @@ struct receiver {
> >>> struct hlist_node list;
> >>> canid_t can_id;
> >>> canid_t mask;
> >>> + bool match_sk;
> >>> unsigned long matches;
> >>> void (*func)(struct sk_buff *skb, void *data);
> >>> void *data;
> >>> diff --git a/net/can/bcm.c b/net/can/bcm.c
> >>> index 909b9e684e04..d89dd82c2178 100644
> >>> --- a/net/can/bcm.c
> >>> +++ b/net/can/bcm.c
> >>> @@ -729,7 +729,8 @@ static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
> >>> {
> >>> if (op->rx_reg_dev == dev) {
> >>> can_rx_unregister(dev_net(dev), dev, op->can_id,
> >>> - REGMASK(op->can_id), bcm_rx_handler, op);
> >>> + REGMASK(op->can_id), false, bcm_rx_handler,
> >>> + op);
> >>>
> >>> /* mark as removed subscription */
> >>> op->rx_reg_dev = NULL;
> >>> @@ -775,6 +776,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
> >>> can_rx_unregister(sock_net(op->sk), NULL,
> >>> op->can_id,
> >>> REGMASK(op->can_id),
> >>> + false,
> >>> bcm_rx_handler, op);
> >>>
> >>> list_del(&op->list);
> >>> @@ -1193,6 +1195,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> >>> err = can_rx_register(sock_net(sk), dev,
> >>> op->can_id,
> >>> REGMASK(op->can_id),
> >>> + false,
> >>> bcm_rx_handler, op,
> >>> "bcm", sk);
> >>>
> >>> @@ -1202,7 +1205,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> >>>
> >>> } else
> >>> err = can_rx_register(sock_net(sk), NULL, op->can_id,
> >>> - REGMASK(op->can_id),
> >>> + REGMASK(op->can_id), false,
> >>> bcm_rx_handler, op, "bcm", sk);
> >>> if (err) {
> >>> /* this bcm rx op is broken -> remove it */
> >>> @@ -1500,7 +1503,7 @@ static int bcm_release(struct socket *sock)
> >>> }
> >>> } else
> >>> can_rx_unregister(net, NULL, op->can_id,
> >>> - REGMASK(op->can_id),
> >>> + REGMASK(op->can_id), false,
> >>> bcm_rx_handler, op);
> >>>
> >>> bcm_remove_op(op);
> >>> diff --git a/net/can/gw.c b/net/can/gw.c
> >>> index ba4124805602..5dbc7b85e0fc 100644
> >>> --- a/net/can/gw.c
> >>> +++ b/net/can/gw.c
> >>> @@ -567,14 +567,15 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
> >>> static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
> >>> {
> >>> return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id,
> >>> - gwj->ccgw.filter.can_mask, can_can_gw_rcv,
> >>> - gwj, "gw", NULL);
> >>> + gwj->ccgw.filter.can_mask, false,
> >>> + can_can_gw_rcv, gwj, "gw", NULL);
> >>> }
> >>>
> >>> static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
> >>> {
> >>> can_rx_unregister(net, gwj->src.dev, gwj->ccgw.filter.can_id,
> >>> - gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
> >>> + gwj->ccgw.filter.can_mask, false, can_can_gw_rcv,
> >>> + gwj);
> >>> }
> >>>
> >>> static int cgw_notifier(struct notifier_block *nb,
> >>> diff --git a/net/can/isotp.c b/net/can/isotp.c
> >>> index 9f94ad3caee9..44d943bbe0b1 100644
> >>> --- a/net/can/isotp.c
> >>> +++ b/net/can/isotp.c
> >>> @@ -1029,7 +1029,7 @@ static int isotp_release(struct socket *sock)
> >>> if (dev) {
> >>> can_rx_unregister(net, dev, so->rxid,
> >>> SINGLE_MASK(so->rxid),
> >>> - isotp_rcv, sk);
> >>> + false, isotp_rcv, sk);
> >>> dev_put(dev);
> >>> }
> >>> }
> >>> @@ -1111,7 +1111,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> >>> if (do_rx_reg)
> >>> can_rx_register(net, dev, addr->can_addr.tp.rx_id,
> >>> SINGLE_MASK(addr->can_addr.tp.rx_id),
> >>> - isotp_rcv, sk, "isotp", sk);
> >>> + false, isotp_rcv, sk, "isotp", sk);
> >>>
> >>> dev_put(dev);
> >>>
> >>> @@ -1122,7 +1122,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> >>> if (dev) {
> >>> can_rx_unregister(net, dev, so->rxid,
> >>> SINGLE_MASK(so->rxid),
> >>> - isotp_rcv, sk);
> >>> + false, isotp_rcv, sk);
> >>> dev_put(dev);
> >>> }
> >>> }
> >>> @@ -1323,7 +1323,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
> >>> if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST)))
> >>> can_rx_unregister(dev_net(dev), dev, so->rxid,
> >>> SINGLE_MASK(so->rxid),
> >>> - isotp_rcv, sk);
> >>> + false, isotp_rcv, sk);
> >>>
> >>> so->ifindex = 0;
> >>> so->bound = 0;
> >>> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> >>> index da3a7a7bcff2..466a20c76fb6 100644
> >>> --- a/net/can/j1939/main.c
> >>> +++ b/net/can/j1939/main.c
> >>> @@ -177,7 +177,7 @@ static int j1939_can_rx_register(struct j1939_priv *priv)
> >>>
> >>> j1939_priv_get(priv);
> >>> ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> >>> - j1939_can_recv, priv, "j1939", NULL);
> >>> + false, j1939_can_recv, priv, "j1939", NULL);
> >>> if (ret < 0) {
> >>> j1939_priv_put(priv);
> >>> return ret;
> >>> @@ -191,7 +191,7 @@ static void j1939_can_rx_unregister(struct j1939_priv *priv)
> >>> struct net_device *ndev = priv->ndev;
> >>>
> >>> can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> >>> - j1939_can_recv, priv);
> >>> + false, j1939_can_recv, priv);
> >>>
> >>> j1939_priv_put(priv);
> >>> }
> >>> diff --git a/net/can/proc.c b/net/can/proc.c
> >>> index d1fe49e6f16d..d312077832a6 100644
> >>> --- a/net/can/proc.c
> >>> +++ b/net/can/proc.c
> >>> @@ -191,11 +191,12 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
> >>>
> >>> hlist_for_each_entry_rcu(r, rx_list, list) {
> >>> char *fmt = (r->can_id & CAN_EFF_FLAG)?
> >>> - " %-5s %08x %08x %pK %pK %8ld %s\n" :
> >>> - " %-5s %03x %08x %pK %pK %8ld %s\n";
> >>> + " %-5s %08x %08x %c %pK %pK %8ld %s\n" :
> >>> + " %-5s %03x %08x %c %pK %pK %8ld %s\n";
> >>>
> >>> seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
> >>> - r->func, r->data, r->matches, r->ident);
> >>> + r->match_sk ? '*' : ' ', r->func, r->data,
> >>> + r->matches, r->ident);
> >>> }
> >>> }
> >>>
> >>> @@ -206,9 +207,9 @@ static void can_print_recv_banner(struct seq_file *m)
> >>> * ....... 0 tp20
> >>> */
> >>> if (IS_ENABLED(CONFIG_64BIT))
> >>> - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
> >>> + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
> >>> else
> >>> - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
> >>> + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
> >>> }
> >>>
> >>> static int can_stats_proc_show(struct seq_file *m, void *v)
> >>> diff --git a/net/can/raw.c b/net/can/raw.c
> >>> index 139d9471ddcf..acfbae28d451 100644
> >>> --- a/net/can/raw.c
> >>> +++ b/net/can/raw.c
> >>> @@ -187,13 +187,13 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
> >>>
> >>> for (i = 0; i < count; i++) {
> >>> err = can_rx_register(net, dev, filter[i].can_id,
> >>> - filter[i].can_mask,
> >>> + filter[i].can_mask, false,
> >>> raw_rcv, sk, "raw", sk);
> >>> if (err) {
> >>> /* clean up successfully registered filters */
> >>> while (--i >= 0)
> >>> can_rx_unregister(net, dev, filter[i].can_id,
> >>> - filter[i].can_mask,
> >>> + filter[i].can_mask, false,
> >>> raw_rcv, sk);
> >>> break;
> >>> }
> >>> @@ -209,7 +209,7 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev,
> >>>
> >>> if (err_mask)
> >>> err = can_rx_register(net, dev, 0, err_mask | CAN_ERR_FLAG,
> >>> - raw_rcv, sk, "raw", sk);
> >>> + false, raw_rcv, sk, "raw", sk);
> >>>
> >>> return err;
> >>> }
> >>> @@ -222,7 +222,7 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
> >>>
> >>> for (i = 0; i < count; i++)
> >>> can_rx_unregister(net, dev, filter[i].can_id,
> >>> - filter[i].can_mask, raw_rcv, sk);
> >>> + filter[i].can_mask, false, raw_rcv, sk);
> >>> }
> >>>
> >>> static inline void raw_disable_errfilter(struct net *net,
> >>> @@ -233,7 +233,7 @@ static inline void raw_disable_errfilter(struct net *net,
> >>> {
> >>> if (err_mask)
> >>> can_rx_unregister(net, dev, 0, err_mask | CAN_ERR_FLAG,
> >>> - raw_rcv, sk);
> >>> + false, raw_rcv, sk);
> >>> }
> >>>
> >>> static inline void raw_disable_allfilters(struct net *net,
> >>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-05 18:54 ` Erik Flodin
@ 2021-05-06 5:26 ` Oliver Hartkopp
2021-05-09 11:28 ` Erik Flodin
0 siblings, 1 reply; 13+ messages in thread
From: Oliver Hartkopp @ 2021-05-06 5:26 UTC (permalink / raw)
To: Erik Flodin; +Cc: linux-can
On 05.05.21 20:54, Erik Flodin wrote:
> On Wed, 5 May 2021 at 11:15, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> On 05.05.21 10:37, Erik Flodin wrote:
>>
>>>>> Add a new flag to can_rx_register/unregister that, when set, requires
>>>>> that the received sk_buff's owning socket matches the socket pointer
>>>>> given when setting the filter. This makes it possible to set a filter
>>>>> that matches all frames sent on a given socket and nothing else.
>>>>
>>
>> (..)
>>
>>>>
>>>> Wouldn't that already match your requirements?
>>>
>>> I guess that would require me to use two sockets: one for sending
>>> (with RECV_ONLY_OWN_MSGS set) and one for receiving. The problem might
>>> then be how to avoid the second socket seeing the frames sent from the
>>> first, without disabling loopback completly?
>>
>> This is indeed a question of the application layout.
>>
>> From what I understood your main requirement is to double check the
>> outgoing traffic whether is has been sent.
>
> What I would like to have is:
> 1. Be notified when a frame has been sent. I send (from user space) a
> single frame and wait until I get confirmation before sending the next
> to give other nodes on the bus a slot to start sending, even if their
> frames have ID with lower priority.
o_O
Sorry, but I have problems to get behind your use-case.
1. You are sending a frame
2. You get a confirmation
3. You are waiting some time (which is not written above), to give other
nodes a slot??
4. goto 1??
> 2. I don't want to receive my own frames, except to support the use
> case above. But then the kernel tells me that it's mine and I can
> easily ignore it.
> 3. Both be able to run with receive filters set (when that makes
> sense) or use the default catch-all filter in cases where I want to
> receive everything and don't want to enumerate all possible CAN IDs.
> 4. Be able to use cansend and candump on the same host as above.
>
>> To me it makes sense to use a
>> separate socket for sending that triggers the special handling of send
>> acknowledges when receiving frames (on that sending socket).
>>
>> And then use a separate socket to receive frames needed by the application.
>
> To be able to use two sockets I would have to perform filtering in
> user space as well in order to filter out frames sent on one socket
> and received on the other. But for how long should I remember the sent
> frame to be able to filter it out? It might not come at all (in the
> case where there are filters set) and setting a time after which it is
> forgotten, even if synchronized with the confirmation on the sending
> socket, just doesn't feel rock solid.
>
>> In both cases (even with your one-socket solution) you need to implement
>> some more or less complex dispatching in user space when receiving frames.
>
> What I do now is to keep the last sent frame around and when I get a
> frame with MSG_CONFIRM set I can check that it's the one I sent,
> ignore it and continue sending the next frame (if any).
>
> // Erik
>
>>>>>
>>>>> Signed-off-by: Erik Flodin <erik@flodin.me>
>>>>> ---
>>>>> include/linux/can/core.h | 4 ++--
>>>>> net/can/af_can.c | 50 ++++++++++++++++++++++------------------
>>>>> net/can/af_can.h | 1 +
>>>>> net/can/bcm.c | 9 +++++---
>>>>> net/can/gw.c | 7 +++---
>>>>> net/can/isotp.c | 8 +++----
>>>>> net/can/j1939/main.c | 4 ++--
>>>>> net/can/proc.c | 11 +++++----
>>>>> net/can/raw.c | 10 ++++----
>>>>> 9 files changed, 58 insertions(+), 46 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/can/core.h b/include/linux/can/core.h
>>>>> index 5fb8d0e3f9c1..7ee68128dc10 100644
>>>>> --- a/include/linux/can/core.h
>>>>> +++ b/include/linux/can/core.h
>>>>> @@ -48,12 +48,12 @@ extern int can_proto_register(const struct can_proto *cp);
>>>>> extern void can_proto_unregister(const struct can_proto *cp);
>>>>>
>>>>> int can_rx_register(struct net *net, struct net_device *dev,
>>>>> - canid_t can_id, canid_t mask,
>>>>> + canid_t can_id, canid_t mask, bool match_sk,
>>>>> void (*func)(struct sk_buff *, void *),
>>>>> void *data, char *ident, struct sock *sk);
>>>>>
>>>>> extern void can_rx_unregister(struct net *net, struct net_device *dev,
>>>>> - canid_t can_id, canid_t mask,
>>>>> + canid_t can_id, canid_t mask, bool match_sk,
>>>>> void (*func)(struct sk_buff *, void *),
>>>>> void *data);
>>>>>
>>>>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>>>>> index cce2af10eb3e..7b639c121653 100644
>>>>> --- a/net/can/af_can.c
>>>>> +++ b/net/can/af_can.c
>>>>> @@ -414,6 +414,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
>>>>> * @dev: pointer to netdevice (NULL => subscribe from 'all' CAN devices list)
>>>>> * @can_id: CAN identifier (see description)
>>>>> * @mask: CAN mask (see description)
>>>>> + * @match_sk: match socket pointer on received sk_buff (see description)
>>>>> * @func: callback function on filter match
>>>>> * @data: returned parameter for callback function
>>>>> * @ident: string for calling module identification
>>>>> @@ -428,6 +429,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
>>>>> * The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can
>>>>> * filter for error message frames (CAN_ERR_FLAG bit set in mask).
>>>>> *
>>>>> + * If match_sk is true, the received sk_buff's owning socket must also match
>>>>> + * the given socket pointer.
>>>>> + *
>>>>> * The provided pointer to the sk_buff is guaranteed to be valid as long as
>>>>> * the callback function is running. The callback function must *not* free
>>>>> * the given sk_buff while processing it's task. When the given sk_buff is
>>>>> @@ -440,8 +444,9 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
>>>>> * -ENODEV unknown device
>>>>> */
>>>>> int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>>>>> - canid_t mask, void (*func)(struct sk_buff *, void *),
>>>>> - void *data, char *ident, struct sock *sk)
>>>>> + canid_t mask, bool match_sk,
>>>>> + void (*func)(struct sk_buff *, void *), void *data,
>>>>> + char *ident, struct sock *sk)
>>>>> {
>>>>> struct receiver *rcv;
>>>>> struct hlist_head *rcv_list;
>>>>> @@ -468,6 +473,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
>>>>>
>>>>> rcv->can_id = can_id;
>>>>> rcv->mask = mask;
>>>>> + rcv->match_sk = match_sk;
>>>>> rcv->matches = 0;
>>>>> rcv->func = func;
>>>>> rcv->data = data;
>>>>> @@ -503,6 +509,7 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
>>>>> * @dev: pointer to netdevice (NULL => unsubscribe from 'all' CAN devices list)
>>>>> * @can_id: CAN identifier
>>>>> * @mask: CAN mask
>>>>> + * @match_sk: match socket pointer on received sk_buff
>>>>> * @func: callback function on filter match
>>>>> * @data: returned parameter for callback function
>>>>> *
>>>>> @@ -510,8 +517,8 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
>>>>> * Removes subscription entry depending on given (subscription) values.
>>>>> */
>>>>> void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>>>>> - canid_t mask, void (*func)(struct sk_buff *, void *),
>>>>> - void *data)
>>>>> + canid_t mask, bool match_sk,
>>>>> + void (*func)(struct sk_buff *, void *), void *data)
>>>>> {
>>>>> struct receiver *rcv = NULL;
>>>>> struct hlist_head *rcv_list;
>>>>> @@ -535,7 +542,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>>>>> */
>>>>> hlist_for_each_entry_rcu(rcv, rcv_list, list) {
>>>>> if (rcv->can_id == can_id && rcv->mask == mask &&
>>>>> - rcv->func == func && rcv->data == data)
>>>>> + rcv->match_sk == match_sk && rcv->func == func &&
>>>>> + rcv->data == data)
>>>>> break;
>>>>> }
>>>>>
>>>>> @@ -546,8 +554,8 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>>>>> * a warning here.
>>>>> */
>>>>> if (!rcv) {
>>>>> - pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
>>>>> - DNAME(dev), can_id, mask);
>>>>> + pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X%s\n",
>>>>> + DNAME(dev), can_id, mask, match_sk ? " (match sk)" : "");
>>>>> goto out;
>>>>> }
>>>>>
>>>>> @@ -569,10 +577,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>>>>> }
>>>>> EXPORT_SYMBOL(can_rx_unregister);
>>>>>
>>>>> -static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
>>>>> +static inline int deliver(struct sk_buff *skb, struct receiver *rcv)
>>>>> {
>>>>> - rcv->func(skb, rcv->data);
>>>>> - rcv->matches++;
>>>>> + if (!rcv->match_sk || skb->sk == rcv->sk) {
>>>>> + rcv->func(skb, rcv->data);
>>>>> + rcv->matches++;
>>>>> + return 1;
>>>>> + }
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buff *skb)
>>>>> @@ -589,8 +601,7 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
>>>>> /* check for error message frame entries only */
>>>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ERR], list) {
>>>>> if (can_id & rcv->mask) {
>>>>> - deliver(skb, rcv);
>>>>> - matches++;
>>>>> + matches += deliver(skb, rcv);
>>>>> }
>>>>> }
>>>>> return matches;
>>>>> @@ -598,23 +609,20 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
>>>>>
>>>>> /* check for unfiltered entries */
>>>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_ALL], list) {
>>>>> - deliver(skb, rcv);
>>>>> - matches++;
>>>>> + matches += deliver(skb, rcv);
>>>>> }
>>>>>
>>>>> /* check for can_id/mask entries */
>>>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_FIL], list) {
>>>>> if ((can_id & rcv->mask) == rcv->can_id) {
>>>>> - deliver(skb, rcv);
>>>>> - matches++;
>>>>> + matches += deliver(skb, rcv);
>>>>> }
>>>>> }
>>>>>
>>>>> /* check for inverted can_id/mask entries */
>>>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx[RX_INV], list) {
>>>>> if ((can_id & rcv->mask) != rcv->can_id) {
>>>>> - deliver(skb, rcv);
>>>>> - matches++;
>>>>> + matches += deliver(skb, rcv);
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -625,15 +633,13 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
>>>>> if (can_id & CAN_EFF_FLAG) {
>>>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_eff[effhash(can_id)], list) {
>>>>> if (rcv->can_id == can_id) {
>>>>> - deliver(skb, rcv);
>>>>> - matches++;
>>>>> + matches += deliver(skb, rcv);
>>>>> }
>>>>> }
>>>>> } else {
>>>>> can_id &= CAN_SFF_MASK;
>>>>> hlist_for_each_entry_rcu(rcv, &dev_rcv_lists->rx_sff[can_id], list) {
>>>>> - deliver(skb, rcv);
>>>>> - matches++;
>>>>> + matches += deliver(skb, rcv);
>>>>> }
>>>>> }
>>>>>
>>>>> diff --git a/net/can/af_can.h b/net/can/af_can.h
>>>>> index 7c2d9161e224..ea98b10d93e7 100644
>>>>> --- a/net/can/af_can.h
>>>>> +++ b/net/can/af_can.h
>>>>> @@ -52,6 +52,7 @@ struct receiver {
>>>>> struct hlist_node list;
>>>>> canid_t can_id;
>>>>> canid_t mask;
>>>>> + bool match_sk;
>>>>> unsigned long matches;
>>>>> void (*func)(struct sk_buff *skb, void *data);
>>>>> void *data;
>>>>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>>>>> index 909b9e684e04..d89dd82c2178 100644
>>>>> --- a/net/can/bcm.c
>>>>> +++ b/net/can/bcm.c
>>>>> @@ -729,7 +729,8 @@ static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
>>>>> {
>>>>> if (op->rx_reg_dev == dev) {
>>>>> can_rx_unregister(dev_net(dev), dev, op->can_id,
>>>>> - REGMASK(op->can_id), bcm_rx_handler, op);
>>>>> + REGMASK(op->can_id), false, bcm_rx_handler,
>>>>> + op);
>>>>>
>>>>> /* mark as removed subscription */
>>>>> op->rx_reg_dev = NULL;
>>>>> @@ -775,6 +776,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
>>>>> can_rx_unregister(sock_net(op->sk), NULL,
>>>>> op->can_id,
>>>>> REGMASK(op->can_id),
>>>>> + false,
>>>>> bcm_rx_handler, op);
>>>>>
>>>>> list_del(&op->list);
>>>>> @@ -1193,6 +1195,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>>>>> err = can_rx_register(sock_net(sk), dev,
>>>>> op->can_id,
>>>>> REGMASK(op->can_id),
>>>>> + false,
>>>>> bcm_rx_handler, op,
>>>>> "bcm", sk);
>>>>>
>>>>> @@ -1202,7 +1205,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>>>>>
>>>>> } else
>>>>> err = can_rx_register(sock_net(sk), NULL, op->can_id,
>>>>> - REGMASK(op->can_id),
>>>>> + REGMASK(op->can_id), false,
>>>>> bcm_rx_handler, op, "bcm", sk);
>>>>> if (err) {
>>>>> /* this bcm rx op is broken -> remove it */
>>>>> @@ -1500,7 +1503,7 @@ static int bcm_release(struct socket *sock)
>>>>> }
>>>>> } else
>>>>> can_rx_unregister(net, NULL, op->can_id,
>>>>> - REGMASK(op->can_id),
>>>>> + REGMASK(op->can_id), false,
>>>>> bcm_rx_handler, op);
>>>>>
>>>>> bcm_remove_op(op);
>>>>> diff --git a/net/can/gw.c b/net/can/gw.c
>>>>> index ba4124805602..5dbc7b85e0fc 100644
>>>>> --- a/net/can/gw.c
>>>>> +++ b/net/can/gw.c
>>>>> @@ -567,14 +567,15 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>>>>> static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
>>>>> {
>>>>> return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id,
>>>>> - gwj->ccgw.filter.can_mask, can_can_gw_rcv,
>>>>> - gwj, "gw", NULL);
>>>>> + gwj->ccgw.filter.can_mask, false,
>>>>> + can_can_gw_rcv, gwj, "gw", NULL);
>>>>> }
>>>>>
>>>>> static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
>>>>> {
>>>>> can_rx_unregister(net, gwj->src.dev, gwj->ccgw.filter.can_id,
>>>>> - gwj->ccgw.filter.can_mask, can_can_gw_rcv, gwj);
>>>>> + gwj->ccgw.filter.can_mask, false, can_can_gw_rcv,
>>>>> + gwj);
>>>>> }
>>>>>
>>>>> static int cgw_notifier(struct notifier_block *nb,
>>>>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>>>>> index 9f94ad3caee9..44d943bbe0b1 100644
>>>>> --- a/net/can/isotp.c
>>>>> +++ b/net/can/isotp.c
>>>>> @@ -1029,7 +1029,7 @@ static int isotp_release(struct socket *sock)
>>>>> if (dev) {
>>>>> can_rx_unregister(net, dev, so->rxid,
>>>>> SINGLE_MASK(so->rxid),
>>>>> - isotp_rcv, sk);
>>>>> + false, isotp_rcv, sk);
>>>>> dev_put(dev);
>>>>> }
>>>>> }
>>>>> @@ -1111,7 +1111,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>>>>> if (do_rx_reg)
>>>>> can_rx_register(net, dev, addr->can_addr.tp.rx_id,
>>>>> SINGLE_MASK(addr->can_addr.tp.rx_id),
>>>>> - isotp_rcv, sk, "isotp", sk);
>>>>> + false, isotp_rcv, sk, "isotp", sk);
>>>>>
>>>>> dev_put(dev);
>>>>>
>>>>> @@ -1122,7 +1122,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>>>>> if (dev) {
>>>>> can_rx_unregister(net, dev, so->rxid,
>>>>> SINGLE_MASK(so->rxid),
>>>>> - isotp_rcv, sk);
>>>>> + false, isotp_rcv, sk);
>>>>> dev_put(dev);
>>>>> }
>>>>> }
>>>>> @@ -1323,7 +1323,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
>>>>> if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST)))
>>>>> can_rx_unregister(dev_net(dev), dev, so->rxid,
>>>>> SINGLE_MASK(so->rxid),
>>>>> - isotp_rcv, sk);
>>>>> + false, isotp_rcv, sk);
>>>>>
>>>>> so->ifindex = 0;
>>>>> so->bound = 0;
>>>>> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
>>>>> index da3a7a7bcff2..466a20c76fb6 100644
>>>>> --- a/net/can/j1939/main.c
>>>>> +++ b/net/can/j1939/main.c
>>>>> @@ -177,7 +177,7 @@ static int j1939_can_rx_register(struct j1939_priv *priv)
>>>>>
>>>>> j1939_priv_get(priv);
>>>>> ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
>>>>> - j1939_can_recv, priv, "j1939", NULL);
>>>>> + false, j1939_can_recv, priv, "j1939", NULL);
>>>>> if (ret < 0) {
>>>>> j1939_priv_put(priv);
>>>>> return ret;
>>>>> @@ -191,7 +191,7 @@ static void j1939_can_rx_unregister(struct j1939_priv *priv)
>>>>> struct net_device *ndev = priv->ndev;
>>>>>
>>>>> can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
>>>>> - j1939_can_recv, priv);
>>>>> + false, j1939_can_recv, priv);
>>>>>
>>>>> j1939_priv_put(priv);
>>>>> }
>>>>> diff --git a/net/can/proc.c b/net/can/proc.c
>>>>> index d1fe49e6f16d..d312077832a6 100644
>>>>> --- a/net/can/proc.c
>>>>> +++ b/net/can/proc.c
>>>>> @@ -191,11 +191,12 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
>>>>>
>>>>> hlist_for_each_entry_rcu(r, rx_list, list) {
>>>>> char *fmt = (r->can_id & CAN_EFF_FLAG)?
>>>>> - " %-5s %08x %08x %pK %pK %8ld %s\n" :
>>>>> - " %-5s %03x %08x %pK %pK %8ld %s\n";
>>>>> + " %-5s %08x %08x %c %pK %pK %8ld %s\n" :
>>>>> + " %-5s %03x %08x %c %pK %pK %8ld %s\n";
>>>>>
>>>>> seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
>>>>> - r->func, r->data, r->matches, r->ident);
>>>>> + r->match_sk ? '*' : ' ', r->func, r->data,
>>>>> + r->matches, r->ident);
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -206,9 +207,9 @@ static void can_print_recv_banner(struct seq_file *m)
>>>>> * ....... 0 tp20
>>>>> */
>>>>> if (IS_ENABLED(CONFIG_64BIT))
>>>>> - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
>>>>> + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
>>>>> else
>>>>> - seq_puts(m, " device can_id can_mask function userdata matches ident\n");
>>>>> + seq_puts(m, " device can_id can_mask own function userdata matches ident\n");
>>>>> }
>>>>>
>>>>> static int can_stats_proc_show(struct seq_file *m, void *v)
>>>>> diff --git a/net/can/raw.c b/net/can/raw.c
>>>>> index 139d9471ddcf..acfbae28d451 100644
>>>>> --- a/net/can/raw.c
>>>>> +++ b/net/can/raw.c
>>>>> @@ -187,13 +187,13 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
>>>>>
>>>>> for (i = 0; i < count; i++) {
>>>>> err = can_rx_register(net, dev, filter[i].can_id,
>>>>> - filter[i].can_mask,
>>>>> + filter[i].can_mask, false,
>>>>> raw_rcv, sk, "raw", sk);
>>>>> if (err) {
>>>>> /* clean up successfully registered filters */
>>>>> while (--i >= 0)
>>>>> can_rx_unregister(net, dev, filter[i].can_id,
>>>>> - filter[i].can_mask,
>>>>> + filter[i].can_mask, false,
>>>>> raw_rcv, sk);
>>>>> break;
>>>>> }
>>>>> @@ -209,7 +209,7 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev,
>>>>>
>>>>> if (err_mask)
>>>>> err = can_rx_register(net, dev, 0, err_mask | CAN_ERR_FLAG,
>>>>> - raw_rcv, sk, "raw", sk);
>>>>> + false, raw_rcv, sk, "raw", sk);
>>>>>
>>>>> return err;
>>>>> }
>>>>> @@ -222,7 +222,7 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
>>>>>
>>>>> for (i = 0; i < count; i++)
>>>>> can_rx_unregister(net, dev, filter[i].can_id,
>>>>> - filter[i].can_mask, raw_rcv, sk);
>>>>> + filter[i].can_mask, false, raw_rcv, sk);
>>>>> }
>>>>>
>>>>> static inline void raw_disable_errfilter(struct net *net,
>>>>> @@ -233,7 +233,7 @@ static inline void raw_disable_errfilter(struct net *net,
>>>>> {
>>>>> if (err_mask)
>>>>> can_rx_unregister(net, dev, 0, err_mask | CAN_ERR_FLAG,
>>>>> - raw_rcv, sk);
>>>>> + false, raw_rcv, sk);
>>>>> }
>>>>>
>>>>> static inline void raw_disable_allfilters(struct net *net,
>>>>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-06 5:26 ` Oliver Hartkopp
@ 2021-05-09 11:28 ` Erik Flodin
2021-05-10 12:28 ` Oliver Hartkopp
0 siblings, 1 reply; 13+ messages in thread
From: Erik Flodin @ 2021-05-09 11:28 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
On Thu, 6 May 2021 at 07:26, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 05.05.21 20:54, Erik Flodin wrote:
> > On Wed, 5 May 2021 at 11:15, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> On 05.05.21 10:37, Erik Flodin wrote:
> >>
> >>>>> Add a new flag to can_rx_register/unregister that, when set, requires
> >>>>> that the received sk_buff's owning socket matches the socket pointer
> >>>>> given when setting the filter. This makes it possible to set a filter
> >>>>> that matches all frames sent on a given socket and nothing else.
> >>>>
> >>
> >> (..)
> >>
> >>>>
> >>>> Wouldn't that already match your requirements?
> >>>
> >>> I guess that would require me to use two sockets: one for sending
> >>> (with RECV_ONLY_OWN_MSGS set) and one for receiving. The problem might
> >>> then be how to avoid the second socket seeing the frames sent from the
> >>> first, without disabling loopback completly?
> >>
> >> This is indeed a question of the application layout.
> >>
> >> From what I understood your main requirement is to double check the
> >> outgoing traffic whether is has been sent.
> >
> > What I would like to have is:
> > 1. Be notified when a frame has been sent. I send (from user space) a
> > single frame and wait until I get confirmation before sending the next
> > to give other nodes on the bus a slot to start sending, even if their
> > frames have ID with lower priority.
>
> o_O
>
> Sorry, but I have problems to get behind your use-case.
>
> 1. You are sending a frame
> 2. You get a confirmation
> 3. You are waiting some time (which is not written above), to give other
> nodes a slot??
No explicit sleep is needed. The machine is sufficient slow so that
just waiting for the confirmation before sending the next frame is
enough. But if the frames are queued in the socket/device then the
frames are sent back-to-back.
> 4. goto 1??
Yes.
// Erik
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-09 11:28 ` Erik Flodin
@ 2021-05-10 12:28 ` Oliver Hartkopp
2021-05-16 20:02 ` Erik Flodin
0 siblings, 1 reply; 13+ messages in thread
From: Oliver Hartkopp @ 2021-05-10 12:28 UTC (permalink / raw)
To: Erik Flodin; +Cc: linux-can
On 09.05.21 13:28, Erik Flodin wrote:
> On Thu, 6 May 2021 at 07:26, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>>> From what I understood your main requirement is to double check the
>>>> outgoing traffic whether is has been sent.
>>>
>>> What I would like to have is:
>>> 1. Be notified when a frame has been sent. I send (from user space) a
>>> single frame and wait until I get confirmation before sending the next
>>> to give other nodes on the bus a slot to start sending, even if their
>>> frames have ID with lower priority.
>>
>> o_O
>>
>> Sorry, but I have problems to get behind your use-case.
>>
>> 1. You are sending a frame
>> 2. You get a confirmation
>> 3. You are waiting some time (which is not written above), to give other
>> nodes a slot??
>
> No explicit sleep is needed. The machine is sufficient slow so that
> just waiting for the confirmation before sending the next frame is
> enough. But if the frames are queued in the socket/device then the
> frames are sent back-to-back.
Ok, I feel I'm getting behind your requirement ...
You mainly want to throttle the outgoing traffic, to make sure that e.g.
a specific CAN ID is not sent 'back-to'back' and might lead to a DoS
situation on the wire?!?
Do you know about the Linux CAN queuing disciplines that might provide
the solution to your question?
See:
http://rtime.felk.cvut.cz/can/
http://rtime.felk.cvut.cz/can/socketcan-qdisc-final.pdf
A four-slides-intro to the concept can be found here (slides 41ff):
https://wiki.automotivelinux.org/_media/agl-distro/agl2017-socketcan-print.pdf
Best,
Oliver
>
>> 4. goto 1??
>
> Yes.
>
> // Erik
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-10 12:28 ` Oliver Hartkopp
@ 2021-05-16 20:02 ` Erik Flodin
2021-05-17 7:36 ` Oliver Hartkopp
0 siblings, 1 reply; 13+ messages in thread
From: Erik Flodin @ 2021-05-16 20:02 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
Hi again,
Sorry for being slow to reply...
On Mon, 10 May 2021 at 14:28, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
>
>
> On 09.05.21 13:28, Erik Flodin wrote:
> > On Thu, 6 May 2021 at 07:26, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> >>>> From what I understood your main requirement is to double check the
> >>>> outgoing traffic whether is has been sent.
> >>>
> >>> What I would like to have is:
> >>> 1. Be notified when a frame has been sent. I send (from user space) a
> >>> single frame and wait until I get confirmation before sending the next
> >>> to give other nodes on the bus a slot to start sending, even if their
> >>> frames have ID with lower priority.
> >>
> >> o_O
> >>
> >> Sorry, but I have problems to get behind your use-case.
> >>
> >> 1. You are sending a frame
> >> 2. You get a confirmation
> >> 3. You are waiting some time (which is not written above), to give other
> >> nodes a slot??
> >
> > No explicit sleep is needed. The machine is sufficient slow so that
> > just waiting for the confirmation before sending the next frame is
> > enough. But if the frames are queued in the socket/device then the
> > frames are sent back-to-back.
>
> Ok, I feel I'm getting behind your requirement ...
>
> You mainly want to throttle the outgoing traffic, to make sure that e.g.
> a specific CAN ID is not sent 'back-to'back' and might lead to a DoS
> situation on the wire?!?
Yes, so after a frame has been sent, I want to leave the bus idle long
enough to give another device on the bus that has something in its TX
queue an opportunity to send.
> Do you know about the Linux CAN queuing disciplines that might provide
> the solution to your question?
Yes, I came across that when I started my journey in CAN-land, but
unfortunately the kernel I have to use doesn't have support for
traffic control. That's when I started using RECV_OWN_MSGS which
worked until I added filters and here we are now :)
If I get the chance to update the kernel (which would have to happen
before my proposed patch could be used anyway) I should perhaps try to
get tc support instead. That would simplify my application at least.
Thanks for your time!
Best regards,
// Erik
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-16 20:02 ` Erik Flodin
@ 2021-05-17 7:36 ` Oliver Hartkopp
2021-05-17 18:09 ` Erik Flodin
0 siblings, 1 reply; 13+ messages in thread
From: Oliver Hartkopp @ 2021-05-17 7:36 UTC (permalink / raw)
To: Erik Flodin; +Cc: linux-can
On 16.05.21 22:02, Erik Flodin wrote:
>> You mainly want to throttle the outgoing traffic, to make sure that e.g.
>> a specific CAN ID is not sent 'back-to'back' and might lead to a DoS
>> situation on the wire?!?
>
> Yes, so after a frame has been sent, I want to leave the bus idle long
> enough to give another device on the bus that has something in its TX
> queue an opportunity to send.
>
>> Do you know about the Linux CAN queuing disciplines that might provide
>> the solution to your question?
>
> Yes, I came across that when I started my journey in CAN-land, but
> unfortunately the kernel I have to use doesn't have support for
> traffic control. That's when I started using RECV_OWN_MSGS which
> worked until I added filters and here we are now :)
>
> If I get the chance to update the kernel (which would have to happen
> before my proposed patch could be used anyway) I should perhaps try to
> get tc support instead. That would simplify my application at least.
:-D
In any case it seems you need to change some kernel code.
The CAN frame ematch rule is from 2012:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f057bbb6f9ed0fb61ea11105c9ef0ed5ac1a354d
Is your kernel older than 2012? o_O
Or was TC just not enabled?
Best regards,
Oliver
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] can: add support for filtering own messages only
2021-05-17 7:36 ` Oliver Hartkopp
@ 2021-05-17 18:09 ` Erik Flodin
0 siblings, 0 replies; 13+ messages in thread
From: Erik Flodin @ 2021-05-17 18:09 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
On Mon, 17 May 2021 at 09:36, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
>
>
> On 16.05.21 22:02, Erik Flodin wrote:
>
> >> You mainly want to throttle the outgoing traffic, to make sure that e.g.
> >> a specific CAN ID is not sent 'back-to'back' and might lead to a DoS
> >> situation on the wire?!?
> >
> > Yes, so after a frame has been sent, I want to leave the bus idle long
> > enough to give another device on the bus that has something in its TX
> > queue an opportunity to send.
> >
> >> Do you know about the Linux CAN queuing disciplines that might provide
> >> the solution to your question?
> >
> > Yes, I came across that when I started my journey in CAN-land, but
> > unfortunately the kernel I have to use doesn't have support for
> > traffic control. That's when I started using RECV_OWN_MSGS which
> > worked until I added filters and here we are now :)
> >
> > If I get the chance to update the kernel (which would have to happen
> > before my proposed patch could be used anyway) I should perhaps try to
> > get tc support instead. That would simplify my application at least.
>
> :-D
>
> In any case it seems you need to change some kernel code.
>
> The CAN frame ematch rule is from 2012:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f057bbb6f9ed0fb61ea11105c9ef0ed5ac1a354d
>
> Is your kernel older than 2012? o_O
> Or was TC just not enabled?
TC wasn't enabled.
// Erik
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-05-17 18:09 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04 20:35 [PATCH v2 0/2] can: Add CAN_RAW_RECV_OWN_MSGS_ALL socket option Erik Flodin
2021-05-04 20:35 ` [PATCH v2 1/2] can: add support for filtering own messages only Erik Flodin
2021-05-05 6:58 ` Oliver Hartkopp
2021-05-05 8:37 ` Erik Flodin
2021-05-05 9:15 ` Oliver Hartkopp
2021-05-05 18:54 ` Erik Flodin
2021-05-06 5:26 ` Oliver Hartkopp
2021-05-09 11:28 ` Erik Flodin
2021-05-10 12:28 ` Oliver Hartkopp
2021-05-16 20:02 ` Erik Flodin
2021-05-17 7:36 ` Oliver Hartkopp
2021-05-17 18:09 ` Erik Flodin
2021-05-04 20:35 ` [PATCH v2 2/2] can: raw: add CAN_RAW_RECV_OWN_MSGS_ALL socket option Erik Flodin
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.