[PATCH 2/3] arch/mips/net/bpf_jit.c: fix failure check

[PATCH 2/3] arch/mips/net/bpf_jit.c: fix failure check

[Andrey Utkin]

static int pkt_type_offset(void) returned -1 in case of failure, and
actual (positive) offset value in case of success. In the only instance
of its usage, the result was saved to local "unsigned int off" variable,
   which is used in a lot of places in the same (large) function, so
   changing its type could cause many warnings.
There was no signed int variable which could be just used for this case.
There are two possibilities to resolve that: to declare temporary signed
int variable to get the return value from pkt_type_offset(), or to
separate return status from returned offset value. The latter approach
was chosen, however, I am not sure which would be practically optimal.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=80371
Reported-by: David Binderman <dcb314@hotmail.com>
Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
---
 arch/mips/net/bpf_jit.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index b87390a..47f65d5 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -770,7 +770,7 @@ static u64 jit_get_skb_w(struct sk_buff *skb, unsigned offset)
 #else
 #define PKT_TYPE_MAX	7
 #endif
-static int pkt_type_offset(void)
+static int pkt_type_offset(unsigned int *off_arg)
 {
 	struct sk_buff skb_probe = {
 		.pkt_type = ~0,
@@ -779,8 +779,10 @@ static int pkt_type_offset(void)
 	unsigned int off;
 
 	for (off = 0; off < sizeof(struct sk_buff); off++) {
-		if (ct[off] == PKT_TYPE_MAX)
-			return off;
+		if (ct[off] == PKT_TYPE_MAX) {
+			*off_arg = off;
+			return 0;
+		}
 	}
 	pr_err_once("Please fix pkt_type_offset(), as pkt_type couldn't be found\n");
 	return -1;
@@ -1332,9 +1334,7 @@ jmp_cmp:
 		case BPF_ANC | SKF_AD_PKTTYPE:
 			ctx->flags |= SEEN_SKB;
 
-			off = pkt_type_offset();
-
-			if (off < 0)
+			if (pkt_type_offset(&off))
 				return -1;
 			emit_load_byte(r_tmp, r_skb, off, ctx);
 			/* Keep only the last 3 bits */
-- 
1.8.5.5

Re: [PATCH 2/3] arch/mips/net/bpf_jit.c: fix failure check

[Markos Chandras]

On 07/17/2014 02:27 PM, Andrey Utkin wrote:
> static int pkt_type_offset(void) returned -1 in case of failure, and
> actual (positive) offset value in case of success. In the only instance
> of its usage, the result was saved to local "unsigned int off" variable,
>    which is used in a lot of places in the same (large) function, so
>    changing its type could cause many warnings.
> There was no signed int variable which could be just used for this case.
> There are two possibilities to resolve that: to declare temporary signed
> int variable to get the return value from pkt_type_offset(), or to
> separate return status from returned offset value. The latter approach
> was chosen, however, I am not sure which would be practically optimal.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=80371
> Reported-by: David Binderman <dcb314@hotmail.com>
> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> ---
>  arch/mips/net/bpf_jit.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
> index b87390a..47f65d5 100644
> --- a/arch/mips/net/bpf_jit.c
> +++ b/arch/mips/net/bpf_jit.c
> @@ -770,7 +770,7 @@ static u64 jit_get_skb_w(struct sk_buff *skb, unsigned offset)
>  #else
>  #define PKT_TYPE_MAX	7
>  #endif
> -static int pkt_type_offset(void)
> +static int pkt_type_offset(unsigned int *off_arg)
>  {
>  	struct sk_buff skb_probe = {
>  		.pkt_type = ~0,
> @@ -779,8 +779,10 @@ static int pkt_type_offset(void)
>  	unsigned int off;
>  
>  	for (off = 0; off < sizeof(struct sk_buff); off++) {
> -		if (ct[off] == PKT_TYPE_MAX)
> -			return off;
> +		if (ct[off] == PKT_TYPE_MAX) {
> +			*off_arg = off;
> +			return 0;
> +		}
>  	}
>  	pr_err_once("Please fix pkt_type_offset(), as pkt_type couldn't be found\n");
>  	return -1;
> @@ -1332,9 +1334,7 @@ jmp_cmp:
>  		case BPF_ANC | SKF_AD_PKTTYPE:
>  			ctx->flags |= SEEN_SKB;
>  
> -			off = pkt_type_offset();
> -
> -			if (off < 0)
> +			if (pkt_type_offset(&off))
>  				return -1;
>  			emit_load_byte(r_tmp, r_skb, off, ctx);
>  			/* Keep only the last 3 bits */
> 
Hi,

Thanks for the patch. I would personally prefer to use a new signed int
variable, but I am fine either way.

-- 
markos

Re: [PATCH 2/3] arch/mips/net/bpf_jit.c: fix failure check

[Andrey Utkin]

2014-07-17 16:52 GMT+03:00 Markos Chandras <Markos.Chandras@imgtec.com>:
> Thanks for the patch. I would personally prefer to use a new signed int
> variable, but I am fine either way.

If that's not a problem for performance etc., then i'll resubmit with
new temporary signed variable, because i believe it would be simpler
to comprehend.

-- 
Andrey Utkin

Re: [PATCH 2/3] arch/mips/net/bpf_jit.c: fix failure check

[Markos Chandras]

On 07/17/2014 02:58 PM, Andrey Utkin wrote:
> 2014-07-17 16:52 GMT+03:00 Markos Chandras <Markos.Chandras@imgtec.com>:
>> Thanks for the patch. I would personally prefer to use a new signed int
>> variable, but I am fine either way.
> 
> If that's not a problem for performance etc., then i'll resubmit with
> new temporary signed variable, because i believe it would be simpler
> to comprehend.
> 
Hi Andrey,

Well, this function is executed during the filter compilation so it's
only executed once (or maybe a few times) per filter (assuming the
filter uses the SKF_AD_PKTTYPE opcode at all) so performance is not
really critical at this point. The performance become a critical factor
for the execution of the jitted filter. But yes, avoiding passing the
argument and doing the pointer operation later on could save some
instructions which is both good for compilation performance and also
makes the function simpler as you said.

-- 
markos

[PATCH] arch/mips/net/bpf_jit.c: fix failure check

[Andrey Utkin]

static int pkt_type_offset(void) returned -1 in case of failure, and actual
(positive) offset value in case of success. In the only instance of its usage,
the result was saved to local "unsigned int off" variable, which is used in a
lot of places in the same (large) function, so changing its type could cause
many warnings. So new signed int variable was added.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=80371
Reported-by: David Binderman <dcb314@hotmail.com>
Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
---
 arch/mips/net/bpf_jit.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c
index b87390a..918b341 100644
--- a/arch/mips/net/bpf_jit.c
+++ b/arch/mips/net/bpf_jit.c
@@ -793,6 +793,7 @@ static int build_body(struct jit_ctx *ctx)
 	const struct sock_filter *inst;
 	unsigned int i, off, load_order, condt;
 	u32 k, b_off __maybe_unused;
+	int tmp;
 
 	for (i = 0; i < prog->len; i++) {
 		u16 code;
@@ -1332,9 +1333,9 @@ jmp_cmp:
 		case BPF_ANC | SKF_AD_PKTTYPE:
 			ctx->flags |= SEEN_SKB;
 
-			off = pkt_type_offset();
+			tmp = off = pkt_type_offset();
 
-			if (off < 0)
+			if (tmp < 0)
 				return -1;
 			emit_load_byte(r_tmp, r_skb, off, ctx);
 			/* Keep only the last 3 bits */
-- 
1.8.5.5