Hi Vincent,
On 02.11.22 04:30, Vincent MAILHOL wrote:
I wonder if this check for (priv->ctrlmode & CAN_CTRLMODE_LISTENONLY)
should be moved to another separate helper function
can_dropped_invalid_tx_skb()? which can then be called by every 'real'
CAN driver.
I think that having a can_dropped_invalid_tx_skb() and a
can_dropped_invalid_skb() would be confusing. People will not know
which one to use without looking at the code.
If we split it in two, I would rather put more explicit names for the functions:
* can_virtual_dropped_invalid_skb(): no CAN_CTRLMODE_LISTENONLY
check. To be used by vcan, vxcan and any other interfaces without a
can_priv member.
* can_dropped_invalid_skb(): calls can_virtual_dropped_invalid_skb()
and checks CAN_CTRLMODE_LISTENONLY. To be used by everyone else.
IMO this makes it even more confusing. Especially with the 'virtual' naming.
The can_dropped_invalid_skb() function is implemented in skb.c and
should (only) handle skb-related stuff.
The fact whether a CAN device is in CAN_CTRLMODE_LISTENONLY mode or not
is nothing skb-specific and IMO including linux/can/netlink.h in skb.c
does not fit at all.
The comment for can_dropped_invalid_skb() states:
/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
And not: "Make sure that the skb is allowed to be sent on this interface
according to its configuration"
I would rather suggest to completely revert the current patch content
and add a new (inline) helper in include/linux/can/dev.h for real CAN
interfaces that do both tests.
This function could perform can_dropped_invalid_skb() *and* the check
for CAN_CTRLMODE_LISTENONLY then.
The check for CAN_CTRLMODE_LISTENONLY has not really something to do
with a correct skb and for that reason can_dropped_invalid_skb() is
probably not the best place for it.
Yes and no. For sure, having any of the CAN_CTRLMODE_* flag doesn’t
make an SKB not well formed. But semantically, it is still invalid to
have such SKB in the TX path.
Yes. But it mixes things up IMO.
Also, I am not sure what better place
there could be for this check.
See above.
One other idea: I do not think there is a way to check whether or not
a netdev has its priv allocated. If we could do such a check, we could
use a single function for both the virtual and the ’real’ CAN drivers.
I am thinking of introducing a new flag to keep track of whether or
not priv is allocated. Something like this:
Not sure whether this would help, as a vcan and vxcan have some priv
size too - but no can_priv with all the CAN device netlink stuff.
Best regards,
Oliver
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 791a51e2f5d6..076818169495 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -337,8 +337,6 @@ static bool can_skb_headroom_valid(struct
net_device *dev, struct sk_buff *skb)
/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
{
- struct can_priv *priv = netdev_priv(dev);
-
switch (ntohs(skb->protocol)) {
case ETH_P_CAN:
if (!can_is_can_skb(skb))
@@ -361,10 +359,14 @@ bool can_dropped_invalid_skb(struct net_device
*dev, struct sk_buff *skb)
if (!can_skb_headroom_valid(dev, skb)) {
goto inval_skb;
- } else if (priv->ctrlmode & CAN_CTRLMODE_LISTENONLY) {
- netdev_info_once(dev,
- "interface in listen only mode,
dropping skb\n");
- goto inval_skb;
+ } else if (netdev_has_priv(netdev)) {
+ struct can_priv *priv = netdev_priv(dev);
+
+ if (priv->ctrlmode & CAN_CTRLMODE_LISTENONLY) {
+ netdev_info_once(dev,
+ "interface in listen only
mode, dropping skb\n");
+ goto inval_skb;
+ }
}
return false;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9f42fc871c3b..7658971f97a1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1693,6 +1693,7 @@ enum netdev_priv_flags {
IFF_LIVE_RENAME_OK = 1<<30,
IFF_TX_SKB_NO_LINEAR = BIT_ULL(31),
IFF_CHANGE_PROTO_DOWN = BIT_ULL(32),
+ IFF_HAS_PRIV = BIT_ULL(33),
};
#define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
@@ -2518,6 +2519,11 @@ void dev_net_set(struct net_device *dev, struct net *net)
write_pnet(&dev->nd_net, net);
}
+static inline bool netdev_has_priv(const struct net_device *dev)
+{
+ return dev->priv_flags & IFF_HAS_PRIV;
+}
+
/**
* netdev_priv - access network device private data
* @dev: network device
diff --git a/net/core/dev.c b/net/core/dev.c
index d66c73c1c734..be0b337c174c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10605,6 +10605,9 @@ struct net_device *alloc_netdev_mqs(int
sizeof_priv, const char *name,
dev = PTR_ALIGN(p, NETDEV_ALIGN);
dev->padded = (char *)dev - (char *)p;
+ if (sizeof_priv)
+ dev->priv_flags |= IFF_HAS_PRIV;
+
ref_tracker_dir_init(&dev->refcnt_tracker, 128);
#ifdef CONFIG_PCPU_DEV_REFCNT
dev->pcpu_refcnt = alloc_percpu(int);
Does it make sense?
Yours sincerely,
Vincent Mailhol
