| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7b1644ad-c117-881e-a64f-35b8d8b40ef7@hartkopp.net>
Date: Sun, 15 May 2022 21:17:04 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>,
Marc Kleine-Budde <mkl@...gutronix.de>
Cc: linux-can@...r.kernel.org, linux-kernel@...r.kernel.org,
Max Staudt <max@...as.org>, netdev@...r.kernel.org
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and
can_skb_headroom_valid to skb.c
Hi Vincent,
On 14.05.22 16:16, Vincent Mailhol wrote:
> The functions can_dropped_invalid_skb() and can_skb_headroom_valid()
> grew a lot over the years to a point which it does not make much sense
> to have them defined as static inline in header files. Move those two
> functions to the .c counterpart of skb.h.
>
> can_skb_headroom_valid() only caller being can_dropped_invalid_skb(),
> the declaration is removed from the header. Only
> can_dropped_invalid_skb() gets its symbol exported.
I can see your point but the need for can-dev was always given for
hardware specific stuff like bitrates, TDC, transceivers, whatever.
As there would be more things in slcan (e.g. dev_alloc_skb() could be
unified with alloc_can_skb()) - would it probably make sense to
introduce a new can-skb module that could be used by all CAN
virtual/software interfaces?
Or some other split-up ... any idea?
Best regards,
Oliver
>
> While doing so, do a small cleanup: add brackets around the else block
> in can_dropped_invalid_skb().
>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> ---
> drivers/net/can/dev/skb.c | 58 ++++++++++++++++++++++++++++++++++++++
> include/linux/can/skb.h | 59 +--------------------------------------
> 2 files changed, 59 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 61660248c69e..8b1991130de5 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -252,3 +252,61 @@ struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
> return skb;
> }
> EXPORT_SYMBOL_GPL(alloc_can_err_skb);
> +
> +/* Check for outgoing skbs that have not been created by the CAN subsystem */
> +static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
> +{
> + /* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
> + if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
> + return false;
> +
> + /* af_packet does not apply CAN skb specific settings */
> + if (skb->ip_summed == CHECKSUM_NONE) {
> + /* init headroom */
> + can_skb_prv(skb)->ifindex = dev->ifindex;
> + can_skb_prv(skb)->skbcnt = 0;
> +
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> + /* perform proper loopback on capable devices */
> + if (dev->flags & IFF_ECHO)
> + skb->pkt_type = PACKET_LOOPBACK;
> + else
> + skb->pkt_type = PACKET_HOST;
> +
> + skb_reset_mac_header(skb);
> + skb_reset_network_header(skb);
> + skb_reset_transport_header(skb);
> + }
> +
> + return true;
> +}
> +
> +/* 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)
> +{
> + const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +
> + if (skb->protocol == htons(ETH_P_CAN)) {
> + if (unlikely(skb->len != CAN_MTU ||
> + cfd->len > CAN_MAX_DLEN))
> + goto inval_skb;
> + } else if (skb->protocol == htons(ETH_P_CANFD)) {
> + if (unlikely(skb->len != CANFD_MTU ||
> + cfd->len > CANFD_MAX_DLEN))
> + goto inval_skb;
> + } else {
> + goto inval_skb;
> + }
> +
> + if (!can_skb_headroom_valid(dev, skb))
> + goto inval_skb;
> +
> + return false;
> +
> +inval_skb:
> + kfree_skb(skb);
> + dev->stats.tx_dropped++;
> + return true;
> +}
> +EXPORT_SYMBOL_GPL(can_dropped_invalid_skb);
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index fdb22b00674a..182749e858b3 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -31,6 +31,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
> struct canfd_frame **cfd);
> struct sk_buff *alloc_can_err_skb(struct net_device *dev,
> struct can_frame **cf);
> +bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);
>
> /*
> * The struct can_skb_priv is used to transport additional information along
> @@ -96,64 +97,6 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
> return nskb;
> }
>
> -/* Check for outgoing skbs that have not been created by the CAN subsystem */
> -static inline bool can_skb_headroom_valid(struct net_device *dev,
> - struct sk_buff *skb)
> -{
> - /* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
> - if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
> - return false;
> -
> - /* af_packet does not apply CAN skb specific settings */
> - if (skb->ip_summed == CHECKSUM_NONE) {
> - /* init headroom */
> - can_skb_prv(skb)->ifindex = dev->ifindex;
> - can_skb_prv(skb)->skbcnt = 0;
> -
> - skb->ip_summed = CHECKSUM_UNNECESSARY;
> -
> - /* perform proper loopback on capable devices */
> - if (dev->flags & IFF_ECHO)
> - skb->pkt_type = PACKET_LOOPBACK;
> - else
> - skb->pkt_type = PACKET_HOST;
> -
> - skb_reset_mac_header(skb);
> - skb_reset_network_header(skb);
> - skb_reset_transport_header(skb);
> - }
> -
> - return true;
> -}
> -
> -/* Drop a given socketbuffer if it does not contain a valid CAN frame. */
> -static inline bool can_dropped_invalid_skb(struct net_device *dev,
> - struct sk_buff *skb)
> -{
> - const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> -
> - if (skb->protocol == htons(ETH_P_CAN)) {
> - if (unlikely(skb->len != CAN_MTU ||
> - cfd->len > CAN_MAX_DLEN))
> - goto inval_skb;
> - } else if (skb->protocol == htons(ETH_P_CANFD)) {
> - if (unlikely(skb->len != CANFD_MTU ||
> - cfd->len > CANFD_MAX_DLEN))
> - goto inval_skb;
> - } else
> - goto inval_skb;
> -
> - if (!can_skb_headroom_valid(dev, skb))
> - goto inval_skb;
> -
> - return false;
> -
> -inval_skb:
> - kfree_skb(skb);
> - dev->stats.tx_dropped++;
> - return true;
> -}
> -
> static inline bool can_is_canfd_skb(const struct sk_buff *skb)
> {
> /* the CAN specific type of skb is identified by its data length */
Powered by blists - more mailing lists