netdev - Re: [PATCH net-next 1/2] flow_dissector: Cleanup control flow

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20170901122647.GB4938@vergenet.net>
Date:   Fri, 1 Sep 2017 14:26:49 +0200
From:   Simon Horman <simon.horman@...ronome.com>
To:     Tom Herbert <tom@...ntonium.net>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, alex.popov@...ux.com,
        hannes@...essinduktion.org
Subject: Re: [PATCH net-next 1/2] flow_dissector: Cleanup control flow

Hi Tom,

On Thu, Aug 31, 2017 at 03:22:38PM -0700, Tom Herbert wrote:
> __skb_flow_dissect is riddled with gotos that make discerning the flow,
> debugging, and extending the capability difficult. This patch
> reorganizes things so that we only perform goto's after the two main
> switch statements (no gotos within the cases now). It also eliminates
> several goto labels so that there are only two labels that can be target
> for goto.

I agree that the flow of __skb_flow_dissect() is difficult to follow
but its not obvious that this significant change in terms of loc
takes us to a better place.

Maybe it makes follow-up work easier. If so perhaps it should be motivated
along those lines.

In any case I won't stand in the way of this change but I did want to throw
my 2c worth in.

> 
> Reported-by: Alexander Popov <alex.popov@...ux.com>
> Signed-off-by: Tom Herbert <tom@...ntonium.net>
> ---
>  include/net/flow_dissector.h |   9 ++
>  net/core/flow_dissector.c    | 225 ++++++++++++++++++++++++++++---------------
>  2 files changed, 156 insertions(+), 78 deletions(-)
> 
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index e2663e900b0a..c358c3ff6acc 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -19,6 +19,15 @@ struct flow_dissector_key_control {
>  #define FLOW_DIS_FIRST_FRAG	BIT(1)
>  #define FLOW_DIS_ENCAPSULATION	BIT(2)
>  
> +enum flow_dissect_ret {
> +	FLOW_DISSECT_RET_OUT_GOOD,
> +	FLOW_DISSECT_RET_OUT_BAD,
> +	FLOW_DISSECT_RET_PROTO_AGAIN,
> +	FLOW_DISSECT_RET_IPPROTO_AGAIN,
> +	FLOW_DISSECT_RET_IPPROTO_AGAIN_EH,
> +	FLOW_DISSECT_RET_CONTINUE,
> +};

Minor nit:

My reading is that this patch does not seem to differentiate between the
handling of FLOW_DISSECT_RET_IPPROTO_AGAIN and
FLOW_DISSECT_RET_IPPROTO_AGAIN_EH.  Perhaps it would be better to add
FLOW_DISSECT_RET_IPPROTO_AGAIN_EH in the following patch where it is used.

> +
>  /**
>   * struct flow_dissector_key_basic:
>   * @thoff: Transport header offset

...