netdev - Re: [PATCH bpf] xsk: check IFF

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4c993fb0-de7b-7671-8d0a-19bd7c49e70c@intel.com>
Date:   Wed, 15 Feb 2023 18:57:31 +0100
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>
CC:     <bpf@...r.kernel.org>, <ast@...nel.org>, <daniel@...earbox.net>,
        <andrii@...nel.org>, <netdev@...r.kernel.org>,
        <magnus.karlsson@...el.com>, <bjorn@...nel.org>,
        <michal.kubiak@...el.com>
Subject: Re: [PATCH bpf] xsk: check IFF_UP earlier in Tx path

From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Date: Wed, 15 Feb 2023 15:33:09 +0100

> Xsk Tx can be triggered via either sendmsg() or poll() syscalls. These
> two paths share a call to common function xsk_xmit() which has two
> sanity checks within. A pseudo code example to show the two paths:

[...]

> @@ -627,17 +618,31 @@ static bool xsk_no_wakeup(struct sock *sk)
>  #endif
>  }
>  
> +static int xsk_check_common(struct xdp_sock *xs)
> +{
> +	if (unlikely(!xsk_is_bound(xs)))
> +		return -ENXIO;
> +	if (unlikely(!(xs->dev->flags & IFF_UP)))
> +		return -ENETDOWN;
> +
> +	return 0;
> +}

It's called several times in the code. Have you tried marking it inline
and compare the object code? I'm worrying a bit some beyond-smart
compiler can uninline these 4 lines and slow down things for no reason.

(it's okay to have inlines in C files if proven that not marking them
 hurts a lot)

> +
>  static int __xsk_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
The rest is:

Reviewed-by: Alexander Lobakin <aleksander.lobakin@...el.com>

Thanks,
Olek