netdev - Re: [PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to reflect HW offload status

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: <20170209172223.405b9e2f@cakuba.netronome.com>
Date:   Thu, 9 Feb 2017 17:22:23 -0800
From:   Jakub Kicinski <kubakici@...pl>
To:     Or Gerlitz <ogerlitz@...lanox.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.r.fastabend@...el.com>,
        Amir Vadai <amirva@...ai.me>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to
 reflect HW offload status

On Thu,  9 Feb 2017 16:18:08 +0200, Or Gerlitz wrote:
> Currently there is no way of querying whether a filter is
> offloaded to HW or not when using both policy (no flag).
> 
> Reuse the skip flags to show the insertion status by setting
> the skip_hw flag in case the filter wasn't offloaded.
> 
> Signed-off-by: Or Gerlitz <ogerlitz@...lanox.com>
> ---
>  net/sched/cls_bpf.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index d9c9701..91ba90d 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -185,14 +185,23 @@ static int cls_bpf_offload(struct tcf_proto *tp, struct cls_bpf_prog *prog,
>  			return -EINVAL;
>  		}
>  	} else {
> -		if (!tc_should_offload(dev, tp, prog->gen_flags))
> -			return skip_sw ? -EINVAL : 0;
> +		if (!tc_should_offload(dev, tp, prog->gen_flags)) {
> +			if (tc_skip_sw(prog->gen_flags))
> +				return -EINVAL;
> +			prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
> +			return 0;
> +		}
>  		cmd = TC_CLSBPF_ADD;
>  	}
>  
>  	ret = cls_bpf_offload_cmd(tp, obj, cmd);
> -	if (ret)
> -		return skip_sw ? ret : 0;
> +
> +	if (ret) {
> +		if (skip_sw)
> +			return ret;
> +		prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
> +		return 0;
> +	}
>  
>  	obj->offloaded = true;

In cls_bpf we do store information about whether program is offloaded or
not already (see the @offloaded member).  Could we simplify the code
thanks to this?

I'm obviously all for reporting whether tc objects are offloaded or not
but let me ask perhaps the silly question of why reuse the SKIP_HW flag?
We don't have to worry about flag bits running out, could it be clearer
to users to report whether object is present in HW using a new flag?  Or
even two flags for present/non-present so user doesn't have to ponder
what no flag means (old kernel or not offloaded?). I don't really mind
either way I'm just wondering what the motivation was and maybe how
others feel.