| 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: <f96cd163-0364-4c14-882b-48c3f8e0f05a@hartkopp.net>
Date: Mon, 15 Sep 2025 12:16:40 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Vincent Mailhol <mailhol@...nel.org>,
Marc Kleine-Budde <mkl@...gutronix.de>
Cc: linux-can@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] can: raw: use bitfields to store flags in struct
raw_sock
On 15.09.25 11:23, Vincent Mailhol wrote:
> The loopback, recv_own_msgs, fd_frames and xl_frames fields of struct
> raw_sock just need to store one bit of information.
>
> Declare all those members as a bitfields of type unsigned int and
> width one bit.
>
> Add a temporary variable to raw_setsockopt() and raw_getsockopt() to
> make the conversion between the stored bits and the socket interface.
>
> This reduces struct raw_sock by eight bytes.
>
> Statistics before:
>
> $ pahole --class_name=raw_sock net/can/raw.o
> struct raw_sock {
> struct sock sk __attribute__((__aligned__(8))); /* 0 776 */
>
> /* XXX last struct has 1 bit hole */
>
> /* --- cacheline 12 boundary (768 bytes) was 8 bytes ago --- */
> int bound; /* 776 4 */
> int ifindex; /* 780 4 */
> struct net_device * dev; /* 784 8 */
> netdevice_tracker dev_tracker; /* 792 0 */
> struct list_head notifier; /* 792 16 */
> int loopback; /* 808 4 */
> int recv_own_msgs; /* 812 4 */
> int fd_frames; /* 816 4 */
> int xl_frames; /* 820 4 */
> struct can_raw_vcid_options raw_vcid_opts; /* 824 4 */
> canid_t tx_vcid_shifted; /* 828 4 */
> /* --- cacheline 13 boundary (832 bytes) --- */
> canid_t rx_vcid_shifted; /* 832 4 */
> canid_t rx_vcid_mask_shifted; /* 836 4 */
> int join_filters; /* 840 4 */
> int count; /* 844 4 */
> struct can_filter dfilter; /* 848 8 */
> struct can_filter * filter; /* 856 8 */
> can_err_mask_t err_mask; /* 864 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> struct uniqframe * uniq; /* 872 8 */
>
> /* size: 880, cachelines: 14, members: 20 */
> /* sum members: 876, holes: 1, sum holes: 4 */
> /* member types with bit holes: 1, total: 1 */
> /* forced alignments: 1 */
> /* last cacheline: 48 bytes */
> } __attribute__((__aligned__(8)));
>
> ...and after:
>
> $ pahole --class_name=raw_sock net/can/raw.o
> struct raw_sock {
> struct sock sk __attribute__((__aligned__(8))); /* 0 776 */
>
> /* XXX last struct has 1 bit hole */
>
> /* --- cacheline 12 boundary (768 bytes) was 8 bytes ago --- */
> int bound; /* 776 4 */
> int ifindex; /* 780 4 */
> struct net_device * dev; /* 784 8 */
> netdevice_tracker dev_tracker; /* 792 0 */
> struct list_head notifier; /* 792 16 */
> unsigned int loopback:1; /* 808: 0 4 */
> unsigned int recv_own_msgs:1; /* 808: 1 4 */
> unsigned int fd_frames:1; /* 808: 2 4 */
> unsigned int xl_frames:1; /* 808: 3 4 */
This means that the former data structures (int) are not copied but bits
are set (shifted, ANDed, ORed, etc) right?
So what's the difference in the code the CPU has to process for this
improvement? Is implementing this bitmap more efficient or similar to
copy the (unsigned ints) as-is?
>
> /* XXX 4 bits hole, try to pack */
> /* Bitfield combined with next fields */
>
> struct can_raw_vcid_options raw_vcid_opts; /* 809 4 */
>
> /* XXX 3 bytes hole, try to pack */
>
> canid_t tx_vcid_shifted; /* 816 4 */
> canid_t rx_vcid_shifted; /* 820 4 */
> canid_t rx_vcid_mask_shifted; /* 824 4 */
> int join_filters; /* 828 4 */
> /* --- cacheline 13 boundary (832 bytes) --- */
> int count; /* 832 4 */
> struct can_filter dfilter; /* 836 8 */
>
> /* XXX 4 bytes hole, try to pack */
>
> struct can_filter * filter; /* 848 8 */
> can_err_mask_t err_mask; /* 856 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> struct uniqframe * uniq; /* 864 8 */
>
> /* size: 872, cachelines: 14, members: 20 */
> /* sum members: 860, holes: 3, sum holes: 11 */
> /* sum bitfield members: 4 bits, bit holes: 1, sum bit holes: 4 bits */
> /* member types with bit holes: 1, total: 1 */
> /* forced alignments: 1 */
> /* last cacheline: 40 bytes */
> } __attribute__((__aligned__(8)));
>
> Signed-off-by: Vincent Mailhol <mailhol@...nel.org>
> ---
> net/can/raw.c | 47 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/net/can/raw.c b/net/can/raw.c
> index db21d8a8c54d1b6a25a72c7a9d11d5c94f3187b5..cec580ecd58e36931d1be05716e6beb9c93aa271 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -87,10 +87,10 @@ struct raw_sock {
> struct net_device *dev;
> netdevice_tracker dev_tracker;
> struct list_head notifier;
> - int loopback;
> - int recv_own_msgs;
> - int fd_frames;
> - int xl_frames;
> + unsigned int loopback:1;
> + unsigned int recv_own_msgs:1;
> + unsigned int fd_frames:1;
> + unsigned int xl_frames:1;
> struct can_raw_vcid_options raw_vcid_opts;
> canid_t tx_vcid_shifted;
> canid_t rx_vcid_shifted;
> @@ -560,8 +560,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> struct can_filter sfilter; /* single filter */
> struct net_device *dev = NULL;
> can_err_mask_t err_mask = 0;
> - int fd_frames;
> int count = 0;
> + int flag;
> int err = 0;
>
> if (level != SOL_CAN_RAW)
> @@ -682,44 +682,48 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> break;
>
> case CAN_RAW_LOOPBACK:
> - if (optlen != sizeof(ro->loopback))
> + if (optlen != sizeof(flag))
> return -EINVAL;
>
> - if (copy_from_sockptr(&ro->loopback, optval, optlen))
> + if (copy_from_sockptr(&flag, optval, optlen))
> return -EFAULT;
>
> + ro->loopback = !!flag;
This is obviously an additional effort. Instead it a simple copy the
code makes a copy to an extra variable and then an assignment with bit
(shifting) operations.
Best regards,
Oliver
> break;
>
> case CAN_RAW_RECV_OWN_MSGS:
> - if (optlen != sizeof(ro->recv_own_msgs))
> + if (optlen != sizeof(flag))
> return -EINVAL;
>
> - if (copy_from_sockptr(&ro->recv_own_msgs, optval, optlen))
> + if (copy_from_sockptr(&flag, optval, optlen))
> return -EFAULT;
>
> + ro->recv_own_msgs = !!flag;
> break;
>
> case CAN_RAW_FD_FRAMES:
> - if (optlen != sizeof(fd_frames))
> + if (optlen != sizeof(flag))
> return -EINVAL;
>
> - if (copy_from_sockptr(&fd_frames, optval, optlen))
> + if (copy_from_sockptr(&flag, optval, optlen))
> return -EFAULT;
>
> /* Enabling CAN XL includes CAN FD */
> - if (ro->xl_frames && !fd_frames)
> + if (ro->xl_frames && !flag)
> return -EINVAL;
>
> - ro->fd_frames = fd_frames;
> + ro->fd_frames = !!flag;
> break;
>
> case CAN_RAW_XL_FRAMES:
> - if (optlen != sizeof(ro->xl_frames))
> + if (optlen != sizeof(flag))
> return -EINVAL;
>
> - if (copy_from_sockptr(&ro->xl_frames, optval, optlen))
> + if (copy_from_sockptr(&flag, optval, optlen))
> return -EFAULT;
>
> + ro->xl_frames = !!flag;
> +
> /* Enabling CAN XL includes CAN FD */
> if (ro->xl_frames)
> ro->fd_frames = ro->xl_frames;
> @@ -758,6 +762,7 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
> {
> struct sock *sk = sock->sk;
> struct raw_sock *ro = raw_sk(sk);
> + int flag;
> int len;
> void *val;
>
> @@ -806,25 +811,29 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
> case CAN_RAW_LOOPBACK:
> if (len > sizeof(int))
> len = sizeof(int);
> - val = &ro->loopback;
> + flag = ro->loopback;
> + val = &flag;
> break;
>
> case CAN_RAW_RECV_OWN_MSGS:
> if (len > sizeof(int))
> len = sizeof(int);
> - val = &ro->recv_own_msgs;
> + flag = ro->recv_own_msgs;
> + val = &flag;
> break;
>
> case CAN_RAW_FD_FRAMES:
> if (len > sizeof(int))
> len = sizeof(int);
> - val = &ro->fd_frames;
> + flag = ro->fd_frames;
> + val = &flag;
> break;
>
> case CAN_RAW_XL_FRAMES:
> if (len > sizeof(int))
> len = sizeof(int);
> - val = &ro->xl_frames;
> + flag = ro->xl_frames;
> + val = &flag;
> break;
>
> case CAN_RAW_XL_VCID_OPTS: {
>
Powered by blists - more mailing lists