re: xfrm: policy: add inexact policy search tree infrastructure

[Colin Ian King]

Hi,

Static analysis with CoverityScan found a potential issue with the commit:

commit 6be3b0db6db82cf056a72cc18042048edd27f8ee
Author: Florian Westphal 
Date:   Wed Nov 7 23:00:37 2018 +0100

xfrm: policy: add inexact policy search tree infrastructure

It seems that pointer pol is set to NULL and then a check to see if it
is non-null is used to set pol to tmp; howeverm this check is always
going to be false because pol is always NULL.

The issue is reported by CoverityScan as follows:

Line
1658
assignment: Assigning: pol = NULL.
1659pol = NULL;
1660for (i = 0; i < ARRAY_SIZE(cand.res); i++) {
1661struct xfrm_policy *tmp;
1662
1663tmp = __xfrm_policy_bysel_ctx(cand.res[i], mark,
1664  if_id, type, dir,
1665  sel, ctx);

null: At condition pol, the value of pol must be NULL.
dead_error_condition: The condition pol cannot be true.

CID 1475480 (#1 of 1): Logically dead code

(DEADCODE) dead_error_line: Execution cannot reach the expression
tmp->pos < pol->pos inside this statement: if (tmp && pol && tmp->pos 

1666if (tmp && pol && tmp->pos < pol->pos)
1667pol = tmp;
1668}


I suspect this is not intentional and is probably a bug.

Colin

Re: ipmr, ip6mr: Unite dumproute flows

[Colin Ian King]

Hi,

Static analysis on linux-next has picked up a potential bug with commit:

commit 7b0db85737db3f4d76b2a412e4f19eae59b8b494
Author: Yuval Mintz 
Date:   Wed Feb 28 23:29:39 2018 +0200

ipmr, ip6mr: Unite dumproute flows

in function mr_table_dump(), s_e is and never seems to change, so the
check if (e < s_e) is never true:  See code below, as annotated by
CoverityScan:

317}
   assignment: Assigning: e = 0U.

318e = 0;
   assignment: Assigning: s_e = 0U.

319s_e = 0;
320
321spin_lock_bh(lock);
322list_for_each_entry(mfc, >mfc_unres_queue, list) {
   at_least: At condition e < s_e, the value of e must be at least 0.
   const: At condition e < s_e, the value of s_e must be equal to 0.
   dead_error_condition: The condition e < s_e cannot be true.

323if (e < s_e)

   CID 1474511 (#1 of 1): Logically dead code (DEADCODE)
dead_error_line: Execution cannot reach this statement: goto next_entry2;.

324goto next_entry2;
325if (filter->dev &&
326!mr_mfc_uses_dev(mrt, mfc, filter->dev))
327goto next_entry2;
328
329err = fill(mrt, skb, NETLINK_CB(cb->skb).portid,
330   cb->nlh->nlmsg_seq, mfc, RTM_NEWROUTE, flags);
331if (err < 0) {
332spin_unlock_bh(lock);
333goto out;
334}
335next_entry2:

   incr: Incrementing e. The value of e is now 1.
   incr: Incrementing e. The value of e is now 2.

336e++;


Note that the zero'ing of e and s_e for ipmr and ip6mr was added in by
earlier commits:

commit 8fb472c09b9df478a062eacc7841448e40fc3c17
Author: Nikolay Aleksandrov 
Date:   Thu Jan 12 15:53:33 2017 +0100

ipmr: improve hash scalability

commit 87c418bf1323d57140f4b448715f64de3fbb7e91
Author: Yuval Mintz 
Date:   Wed Feb 28 23:29:31 2018 +0200

ip6mr: Align hash implementation to ipmr

Anyhow, this looks incorrect, but I'm not familiar with this code to
suggest the correct fix.

Colin

Re: [PATCH] net: caif: remove redundant null check on frontpkt

[Colin Ian King]

On 14/09/18 18:54, Sergei Shtylyov wrote:
> Hello!
> 
> On 09/14/2018 08:19 PM, Colin King wrote:
> 
>> From: Colin Ian King 
>>
>> It is impossible for frontpkt to be null at the point of the null
>> check because it has been assigned from rearpkt and there is no
>> way realpkt can be null at the point of the assignment because
> 
>rearpkt?

Good spot. Can this be fixed up when the patch is applied?

> 
>> of the sanity checking and exit paths taken previously. Remove
>> the redundant null check.
>>
>> Detected by CoverityScan, CID#114434 ("Logically dead code")
>>
>> Signed-off-by: Colin Ian King 
> [...]
> 
> MBR, Sergei
> 

Re: [PATCH 1/2] samples: bpf: ensure that we don't load over MAX_PROGS programs

[Colin Ian King]

On 13/07/18 16:11, Dan Carpenter wrote:
> I can't see that we check prog_cnt to ensure it doesn't go over
> MAX_PROGS.
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> index 89161c9ed466..904e775d1a44 100644
> --- a/samples/bpf/bpf_load.c
> +++ b/samples/bpf/bpf_load.c
> @@ -107,6 +107,9 @@ static int load_and_attach(const char *event, struct 
> bpf_insn *prog, int size)
>   return -1;
>   }
>  
> + if (prog_cnt == MAX_PROGS)
> + return -1;
> +

Should that be "if (prog_cnt >= MAX_PROGS)" ?

>   fd = bpf_load_program(prog_type, prog, insns_cnt, license, kern_version,
> bpf_log_buf, BPF_LOG_BUF_SIZE);
>   if (fd < 0) {
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] net/mlx4: fix spelling mistake: "Inrerface" -> "Interface"

[Colin Ian King]

On 22/05/18 16:21, Tariq Toukan wrote:
> 
> 
> On 22/05/2018 11:37 AM, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> Trivial fix to spelling mistake in mlx4_dbg debug message
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/intf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/intf.c
>> b/drivers/net/ethernet/mellanox/mlx4/intf.c
>> index 2edcce98ab2d..6bd4103265d2 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/intf.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/intf.c
>> @@ -172,7 +172,7 @@ int mlx4_do_bond(struct mlx4_dev *dev, bool enable)
>>   list_add_tail(_ctx->list, >ctx_list);
>>   spin_unlock_irqrestore(>ctx_lock, flags);
>>   -    mlx4_dbg(dev, "Inrerface for protocol %d restarted with
>> when bonded mode is %s\n",
>> +    mlx4_dbg(dev, "Interface for protocol %d restarted with when
>> bonded mode is %s\n",
> 
> Thanks Colin.
> I think there's one more thing to fix here.
> It is redundant to say "with when", it was probably done by mistake.
> Let's rephrase, maybe this way?
> 
> restarted with bonded mode %s

Sounds like a good idea, do you want me to send V2 of the patch with
this fix?

> 
>>    dev_ctx->intf->protocol, enable ?
>>    "enabled" : "disabled");
>>   }
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mlxsw: spectrum_switchdev: remove duplicated check on multicast_enable

[Colin Ian King]

On 24/04/18 13:51, Ido Schimmel wrote:
> On Tue, Apr 24, 2018 at 12:51:39PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> The check on bridge_port->bridge_device->multicast_enable is performed
>> twice in two nested if statements. I can't see any reason for this, the
>> second check is redundant and can be removed.
>>
>> Detected by cppcheck:
>> drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c:1722]:
>> (warning) Identical inner 'if' condition is always true.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 9 +++--
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c 
>> b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
>> index c11c9a635866..989ed19e25c9 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
>> @@ -1719,12 +1719,9 @@ __mlxsw_sp_port_mdb_del(struct mlxsw_sp_port 
>> *mlxsw_sp_port,
>>  int err;
>>  
>>  if (bridge_port->bridge_device->multicast_enabled) {
>> -if (bridge_port->bridge_device->multicast_enabled) {
> 
> This looks like a copy-paste error. I believe the intention was to check
> '!bridge_port->mrouter' so that if port is an mrouter port packets
> hitting the MDB entry would still be sent to it even after it was
> removed from the MDB entry ports list.

Ah, that makes a lot more sense.

> 
> I will send a patch later this week after I run it by Nogah who worked
> on this part.

OK, thanks!
> 
> Thanks for the patch.
> 
>> -err = mlxsw_sp_port_smid_set(mlxsw_sp_port, mid->mid,
>> - false);
>> -if (err)
>> -netdev_err(dev, "Unable to remove port from 
>> SMID\n");
>> -}
>> +err = mlxsw_sp_port_smid_set(mlxsw_sp_port, mid->mid, false);
>> +if (err)
>> +netdev_err(dev, "Unable to remove port from SMID\n");
>>  }
>>  
>>  err = mlxsw_sp_port_remove_from_mid(mlxsw_sp_port, mid);
>> -- 
>> 2.17.0
>>

NAK: [PATCH] qed: fix spelling mistake: "checksumed" -> "checksummed"

[Colin Ian King]

On 29/03/18 12:59, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Trivial fix to spelling mistake in DP_INFO message text
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_ll2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c 
> b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> index c4f14fdc4e77..0c50ed1955c4 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> @@ -2383,7 +2383,7 @@ static int qed_ll2_start_xmit(struct qed_dev *cdev, 
> struct sk_buff *skb)
>   u8 flags = 0;
>  
>   if (unlikely(skb->ip_summed != CHECKSUM_NONE)) {
> - DP_INFO(cdev, "Cannot transmit a checksumed packet\n");
> + DP_INFO(cdev, "Cannot transmit a checksummed packet\n");
>   return -EINVAL;
>   }
>  
> 
Found some more issues, I'll send an updated fix in a while

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Colin Ian King]

On 18/02/18 16:31, Joe Perches wrote:
> On Sun, 2018-02-18 at 16:58 +0200, Andy Shevchenko wrote:
>> On Fri, Feb 16, 2018 at 6:53 PM, Colin Ian King
>> <colin.k...@canonical.com> wrote:
>>> On 16/02/18 16:51, Andy Shevchenko wrote:
>>>> On Thu, Feb 15, 2018 at 9:42 PM, Colin King <colin.k...@canonical.com> 
>>>> wrote:
>>>>> +   filter->f.mask.tcp_spec.dst_ip[i] |=
>>>>> 
>>>>> cpu_to_be32(0x);
>>>>
>>>> Can it be one line then?
>>>
>>> I re-adjusted the text because checkpatch was complaining.
>>>>
>>>>> +   filter->f.mask.tcp_spec.src_ip[i] |=
>>>>> 
>>>>> cpu_to_be32(0x);
>>
>> For the rest OK, but for the above two how much over 80 it went if
>> would be one line?
>> If it 2-3 characters, consider to make it one line. It would increase
>> readability.
> 
> Another possibility would be to use temporaries for
>   filter->f.mask.tcp_spec
> and
>   filter->f.data.tcp_spec
> as both are used ~10 times in the function

That's a good idea. I'll fix this up tomorrow when I get back to work
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] i40evf: remove redundant array comparisons to 0 checks

[Colin Ian King]

On 16/02/18 16:51, Andy Shevchenko wrote:
> On Thu, Feb 15, 2018 at 9:42 PM, Colin King <colin.k...@canonical.com> wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> The checks to see if key->dst.s6_addr and key->src.s6_addr are null
>> pointers are redundant because these are constant size arrays and
>> so the checks always return true.  Fix this by removing the redundant
>> checks.
> 
>> +   for (i = 0; i < 4; i++)
> 
>> +   filter->f.mask.tcp_spec.dst_ip[i] |=
>> 
>> cpu_to_be32(0x);
> 
> Can it be one line then?

I re-adjusted the text because checkpatch was complaining.

> 
>> +   memcpy(>f.data.tcp_spec.dst_ip,
>> +  >dst.s6_addr32,
> 
> Ditto.
> 
>> +  sizeof(filter->f.data.tcp_spec.dst_ip));
>> +
>> +   for (i = 0; i < 4; i++)
> 
>> +   filter->f.mask.tcp_spec.src_ip[i] |=
>> 
>> cpu_to_be32(0x);
> 
> Ditto.
> 
>> +   memcpy(>f.data.tcp_spec.src_ip,
>> +  >src.s6_addr32,
> 
> Ditto.
> 
>> +  sizeof(filter->f.data.tcp_spec.src_ip));
> 

Re: [PATCH][next] bnxt_en: ensure len is ininitialized to zero

[Colin Ian King]

On 12/01/18 22:38, Andy Gospodarek wrote:
> On Fri, Jan 12, 2018 at 10:11:17AM -0800, Michael Chan wrote:
>> On Fri, Jan 12, 2018 at 9:46 AM, Colin King <colin.k...@canonical.com> wrote:
>>> From: Colin Ian King <colin.k...@canonical.com>
>>>
>>> In the case where cmp_type == CMP_TYPE_RX_L2_TPA_START_CMP the
>>> exit return path is via label next_rx_no_prod and cpr->rx_bytes
>>> is being updated by an uninitialized value from len. Fix this by
>>> initializing len to zero.
>>>
>>> Detected by CoverityScan, CID#1463807 ("Uninitialized scalar variable")
>>>
>>> Fixes: 6a8788f25625 ("bnxt_en: add support for software dynamic interrupt 
>>> moderation")
>>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>>> ---
>>>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c 
>>> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> index cf6ebf1e324b..5b5c4f266f1b 100644
>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> @@ -1482,7 +1482,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct 
>>> bnxt_napi *bnapi, u32 *raw_cons,
>>> u32 tmp_raw_cons = *raw_cons;
>>> u16 cfa_code, cons, prod, cp_cons = RING_CMP(tmp_raw_cons);
>>> struct bnxt_sw_rx_bd *rx_buf;
>>> -   unsigned int len;
>>> +   unsigned int len = 0;
>>
>> It might be better to add a new label next_rx_no_prod_no_len and have
>> the TPA code paths jump there instead.
>>
>> Andy, what do you think?
>>
> 
> Yes, I think that would be a better fix.  Making the TPA vs not-TPA as
> explicit and intentional as possible seems like a good idea.

OK, new patch sent.
> 
>>> u8 *data_ptr, agg_bufs, cmp_type;
>>> dma_addr_t dma_addr;
>>> struct sk_buff *skb;
>>> --
>>> 2.15.1
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH][next] wcn36xx: remove redundant assignment to msg_body.min_ch_time

[Colin Ian King]

On 29/12/17 07:44, Loic Poulain wrote:
> Hi Colin, Bjorn,
> 
> On 26 December 2017 at 21:13, Bjorn Andersson
> <bjorn.anders...@linaro.org> wrote:
>> On Tue 19 Dec 09:04 PST 2017, Colin King wrote:
>>
>>> From: Colin Ian King <colin.k...@canonical.com>
>>>
>>> msg_body.min_ch_time is being assigned twice; remove the redundant
>>> first assignment.
>>>
>>> Detected by CoverityScan, CID#1463042 ("Unused Value")
>>>
>>
>> Happy to see Coverity working for us :)
>>
>>
>> This should have had a:
>>
>> Fixes: 2f3bef4b247e ("wcn36xx: Add hardware scan offload support")
>>
>>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>>> ---
>>>  drivers/net/wireless/ath/wcn36xx/smd.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c 
>>> b/drivers/net/wireless/ath/wcn36xx/smd.c
>>> index 2914618a0335..bab2eca5fcac 100644
>>> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
>>> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
>>> @@ -625,7 +625,6 @@ int wcn36xx_smd_start_hw_scan(struct wcn36xx *wcn, 
>>> struct ieee80211_vif *vif,
>>>   INIT_HAL_MSG(msg_body, WCN36XX_HAL_START_SCAN_OFFLOAD_REQ);
>>>
>>>   msg_body.scan_type = WCN36XX_HAL_SCAN_TYPE_ACTIVE;
>>> - msg_body.min_ch_time = 30;
>>>   msg_body.min_ch_time = 100;
>>
>> But I strongly suspect the second line is supposed to be max_ch_time.
>>
>> @Loic, do you agree?
> 
> You're absolutely right.
> Colin could you please update your patch accordingly?

Resent as "wcn36xx: fix incorrect assignment to msg_body.min_ch_time"

> 
> Regards,
> Loic
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] atm: lanai: use setup_timer instead of init_timer

[Colin Ian King]

On 30/11/17 14:23, David Miller wrote:
> From: Colin King <colin.k...@canonical.com>
> Date: Fri, 24 Nov 2017 13:27:12 +
> 
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> Use setup_timer function instead of initializing timer with the
>> function and data fields.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> 
> setup_timer() == -ENOEXIST.
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Apologies. My bad.

Colin

Re: [PATCH][net-next] ipv6: fix incorrect bitwise operator used on rt6i_flags

[Colin Ian King]

On 10/10/17 19:23, Wei Wang wrote:
> On Tue, Oct 10, 2017 at 11:10 AM, Martin KaFai Lau <ka...@fb.com> wrote:
>> On Tue, Oct 10, 2017 at 05:55:27PM +, Colin King wrote:
>>> From: Colin Ian King <colin.k...@canonical.com>
>>>
>>> The use of the | operator always leads to true on the expression
>>> (rt->rt6i_flags | RTF_CACHE) which looks rather suspect to me. I
>>> believe this is fixed by using & instead to just check the
>>> RTF_CACHE entry bit.
>> Good catch. LGTM. If rt does not have RTF_CACHE set, it should not be in the
>> exception table.
>>
>> Acked-by: Martin KaFai Lau <ka...@fb.com>
>>
> 
> Thanks a lot for catching this. Yes. It should have been '&' instead of '|'.
> 
> Acked-by: Wei Wang <wei...@google.com>

Sorry, can you look at V2 of this patch; there is one more occurrence
that needed fixing.


> 
>>>
>>> Detected by CoverityScan, CID#1457747 ("Wrong operator used")
>>>
>>> Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
>>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>>> ---
>>>  net/ipv6/route.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index 6db1541eaa7b..0556d1ee189c 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
>>>   int err;
>>>
>>>   if (!from ||
>>> - !(rt->rt6i_flags | RTF_CACHE))
>>> + !(rt->rt6i_flags & RTF_CACHE))
>>>   return -EINVAL;
>>>
>>>   if (!rcu_access_pointer(from->rt6i_exception_bucket))
>>> --
>>> 2.14.1
>>>

NACK: [PATCH][net-next] ipv6: fix incorrect bitwise operator used on rt6i_flags

[Colin Ian King]

On 10/10/17 18:55, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> The use of the | operator always leads to true on the expression
> (rt->rt6i_flags | RTF_CACHE) which looks rather suspect to me. I
> believe this is fixed by using & instead to just check the
> RTF_CACHE entry bit.
> 
> Detected by CoverityScan, CID#1457747 ("Wrong operator used")
> 
> Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  net/ipv6/route.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 6db1541eaa7b..0556d1ee189c 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
>   int err;
>  
>   if (!from ||
> - !(rt->rt6i_flags | RTF_CACHE))
> + !(rt->rt6i_flags & RTF_CACHE))
>   return -EINVAL;
>  
>   if (!rcu_access_pointer(from->rt6i_exception_bucket))
> 
Nack that, seems like this occurs more than once and I failed to spot
the others.

Re: [PATCH] e1000: avoid null pointer dereference on invalid stat type

[Colin Ian King]

On 22/09/17 12:50, Dan Carpenter wrote:
> On Thu, Sep 21, 2017 at 11:01:58PM +0100, Colin King wrote:
>> @@ -1837,12 +1838,13 @@ static void e1000_get_ethtool_stats(struct 
>> net_device *netdev,
>>  p = (char *)adapter + stat->stat_offset;
>>  break;
>>  default:
>> +p = NULL;
>>  WARN_ONCE(1, "Invalid E1000 stat type: %u index %d\n",
>>stat->type, i);
>>  break;
>>  }
>>  
>> -if (stat->sizeof_stat == sizeof(u64))
>> +if (p && stat->sizeof_stat == sizeof(u64))
>>  data[i] = *(u64 *)p;
>>  else
>>  data[i] = *(u32 *)p;
>
> 
> The else side will still crash.
> 
> regards,
> dan carpenter
> 
Thanks, stupid me. I'll fix that up.

Re: [PATCH] wireless: iwlegacy: make const array static to shink object code size Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit

[Colin Ian King]

On 22/09/17 11:03, Joe Perches wrote:
> On Fri, 2017-09-22 at 09:23 +0200, Julia Lawall wrote:
>>
>> On Thu, 21 Sep 2017, Colin King wrote:
>>
>>> From: Colin Ian King <colin.k...@canonical.com>
>>>
>>> Don't populate const array ac_to_fifo on the stack in an inlined
>>> function, instead make it static.  Makes the object code smaller
>>> by over 800 bytes:
>>>
>>>textdata bss dec hex filename
>>>  159029   331541216  193399   2f377 4965-mac.o
>>>
>>>textdata bss dec hex filename
>>>  158122   332501216  192588   2f04c 4965-mac.o
>>>
>>> (gcc version 7.2.0 x86_64)
>>
>> Gcc 7 must be much more aggressive about inlining than gcc 4.  Here is the
>> change that I got on this file:
>>
>>  text  data bss dec hex filename
>> -   76346  1494 152   77992   130a8 
>> drivers/net/wireless/intel/iwlegacy/4965-mac.o
>> +   76298  1494 152   77944   13078 
>> drivers/net/wireless/intel/iwlegacy/4965-mac.o
>> decrease of 48
> 
> More likely different CONFIG options.
> 
> e.g. allyesconfig vs defconfig with wireless
> 
yup, I used allyesconfig

re: mac80211: avoid allocating TXQs that won't be used

[Colin Ian King]

Johannes,

Static analysis with CoverityScan on linux-next today detected a null
pointer dereference issue on commit:

>From 0fc4b3403d215ecd3c05505ec1f0028a227ed319 Mon Sep 17 00:00:00 2001
From: Johannes Berg 
Date: Thu, 22 Jun 2017 12:20:29 +0200
Subject: [PATCH] mac80211: avoid allocating TXQs that won't be used

Issue: sdata is null when the sdata is dereferenced by:

   sdata->vif.type != NL80211_IFTYPE_AP_VLAN &&
   sdata->vif.type != NL80211_IFTYPE_MONITOR)

note that sdata is assigned a non-null much later with the statement
sdata = netdev_priv(ndev).

Detected by CoverityScan CID#1456974 ("Explicit null dereferenced")

Colin

Re: [PATCH][next][V2] bpf: test_maps: fix typo "conenct" -> "connect"

[Colin Ian King]

On 30/08/17 14:46, Daniel Borkmann wrote:
> On 08/30/2017 01:47 PM, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> Trivial fix to typo in printf error message
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> 
> For net-next; looks like there is also one in "failed to listeen\n".
> Want to fix this one as well ? ;)

Ah, missed that one. Thanks.
> 
> Acked-by: Daniel Borkmann <dan...@iogearbox.net>

Re: [PATCH][next] net: hinic: fix comparison of a uint16_t type with -1

[Colin Ian King]

On 24/08/17 09:48, Aviad Krawczyk wrote:
> On 8/23/2017 6:39 PM, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> The comparison of hw_ioctxt.rx_buf_sz_idx == -1 is always false because
>> rx_buf_sz_idx is a uint16_t. Fix this by explicitly casting -1 to uint16_t.
>>
>> Detected by CoverityScan, CID#1454559 ("Operands don't affect result")
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c 
>> b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
>> index 09dec6de8dd5..71e26070fb7f 100644
>> --- a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
>> +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
>> @@ -352,7 +352,7 @@ static int set_hw_ioctxt(struct hinic_hwdev *hwdev, 
>> unsigned int rq_depth,
>>  }
>>  }
>>  
>> -if (hw_ioctxt.rx_buf_sz_idx == -1)
>> +if (hw_ioctxt.rx_buf_sz_idx == (uint16_t)-1)
>>  return -EINVAL;
>>  
>>  hw_ioctxt.sq_depth  = ilog2(sq_depth);
>>
> 
> Many thanks, Colin.
> I prefer to avoid casting when possible, what do you think about replacing 
> the condition by:
> 
> if (rx_buf_sz_table[i].sz != HINIC_RX_BUF_SZ)
>   return -EINVAL;
> 

Does that work as expected when rx_buf_sz_table[i].sz == -1?

Re: [PATCH] mlx5: ensure 0 is returned when vport is zero

[Colin Ian King]

On 18/08/17 16:25, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Currently, if vport is zero then then an uninialized return status
> in err is returned.  Since the only return status at the end of the
> function esw_add_uc_addr is zero for the current set of return paths
> we may as well just return 0 rather than err to fix this issue.
> 
> Detected by CoverityScan, CID#1452698 ("Uninitialized scalar variable")
> 
> Fixes: eeb66cdb6826 ("net/mlx5: Separate between E-Switch and MPFS")
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c 
> b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> index 6d9fb6ac6e9b..c77f4c0c7769 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> @@ -401,7 +401,7 @@ static int esw_add_uc_addr(struct mlx5_eswitch *esw, 
> struct vport_addr *vaddr)
>   esw_debug(esw->dev, "\tADDED UC MAC: vport[%d] %pM fr(%p)\n",
> vport, mac, vaddr->flow_rule);
>  
> - return err;
> + return 0;
>  }
>  
>  static int esw_del_uc_addr(struct mlx5_eswitch *esw, struct vport_addr 
> *vaddr)
> 
Apologies, I accidentally re-sent this by mistake.

Re: [PATCH][V2] net/mlx4: fix spelling mistake: "availible" -> "available"

[Colin Ian King]

On 16/08/17 14:58, Leon Romanovsky wrote:
> On Wed, Aug 16, 2017 at 02:42:50PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> Trivial fix to spelling mistakes in the mlx4 driver.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  drivers/net/ethernet/mellanox/mlx4/cmd.c| 16 
>>  drivers/net/ethernet/mellanox/mlx4/fw_qos.c |  6 +++---
>>  drivers/net/ethernet/mellanox/mlx4/fw_qos.h | 10 +-
>>  3 files changed, 16 insertions(+), 16 deletions(-)
>>
> 
> What are the changes between this version and previous one?
> 
> Thanks
> 
A fix on "Availible"

-   mlx4_dbg(dev, "Port %d Availible VPPs %d\n", port, availible_vpp);
+   mlx4_dbg(dev, "Port %d Available VPPs %d\n", port, available_vpp);

Colin



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] netfilter: fix indent on in statements

[Colin Ian King]

On 15/08/17 10:45, Sergei Shtylyov wrote:
> Hello!
> 
> On 8/15/2017 9:50 AM, Colin King wrote:
> 
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> The returns on some if statements are not indented correctly,
> 
>s/in/if/ in the subject?

Doh, fix resent.

> 
>> add in the missing tab.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> [...]
> 
> MBR, Sergei
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

NACK: [PATCH] rtlwifi: btcoex: make function btc8723b2ant_dac_swing static

[Colin Ian King]

On 12/08/17 23:00, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> The function btc8723b2ant_dac_swing  is local to the source and
> does not need to be in global scope, so make it static.
> 
> Cleans up sparse warning:
> symbol 'btc8723b2ant_dac_swing' was not declared. Should it be static?
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c 
> b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c
> index 31965f0ef69d..9d2ecfbc7b33 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtc8723b2ant.c
> @@ -833,9 +833,9 @@ static void btc8723b2ant_set_sw_fulltime_dac_swing(struct 
> btc_coexist *btcoex,
>   btc8723b2ant_set_dac_swing_reg(btcoex, 0x18);
>  }
>  
> -void btc8723b2ant_dac_swing(struct btc_coexist *btcoexist,
> - bool force_exec, bool dac_swing_on,
> - u32 dac_swing_lvl)
> +static void btc8723b2ant_dac_swing(struct btc_coexist *btcoexist,
> +bool force_exec, bool dac_swing_on,
> +u32 dac_swing_lvl)
>  {
>   struct rtl_priv *rtlpriv = btcoexist->adapter;
>  
> 
Sent out wrong patch. Please ignore.

Re: [PATCH] rt2x00: make const array glrt_table static

[Colin Ian King]

On 12/07/17 07:49, Stanislaw Gruszka wrote:
> On Tue, Jul 11, 2017 at 12:47:33PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> Don't populate array glrt_table on the stack but make it static.
>> Makes the object code a smaller by over 670 bytes:
>>
>> Before:
>>text data bss dec hex filename
>>  131772 4733   0  136505   21539 rt2800lib.o
>>
>> After:
>>text data bss dec hex filename
>>  131043 4789   0  135832   21298 rt2800lib.o
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> 
> Acked-by: Stanislaw Gruszka <sgrus...@redhat.com>
> 
> I wonder why compiler do not optimize by itself since array is
> const, but patch is ok.

Afraid marking it as const does not guarantee that at all. The const
qualifier just announces that the value will not be changed [1]. So one
requires static const to ensure it's not populated on the stack and also
marked as non-modifiable.

[1] Section A4.4, The C programming Language, page 196

Colin

> 
> Stanislaw 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] net/mlx4: fix spelling mistake: "enforcment" -> "enforcement"

[Colin Ian King]

On 27/06/17 11:33, Tariq Toukan wrote:
> 
> 
> On 27/06/2017 1:02 PM, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> Trivial fix to spelling mistake in mlx4_dbg debug message
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/cmd.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c
>> b/drivers/net/ethernet/mellanox/mlx4/cmd.c
>> index c1af47e45d3f..9e4c142c7ecd 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/cmd.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c
>> @@ -3280,7 +3280,7 @@ int mlx4_set_vf_link_state(struct mlx4_dev *dev,
>> int port, int vf, int link_stat
>> if (mlx4_master_immediate_activate_vlan_qos(priv, slave, port))
>>   mlx4_dbg(dev,
>> - "updating vf %d port %d no link state HW enforcment\n",
>> + "updating vf %d port %d no link state HW enforecment\n",
> Hi Colin,
> You still have a typo. It's "enforcement".
>>vf, port);
>>   return 0;
>>   }
>>
Doh, stupid me. V2 fixes this.

Re: [PATCH] batman-adv: fix spelling mistake "ourselve" -> "ourself"

[Colin Ian King]

On 26/06/17 11:02, Sergei Shtylyov wrote:
> On 6/26/2017 1:01 PM, Sergei Shtylyov wrote:
> 
>>> From: Colin Ian King <colin.k...@canonical.com>
>>>
>>> Trivial fix to spelling mistake in batadv_dbg debug message
>>>
>>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>>> ---
>>>   net/batman-adv/bat_v_ogm.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c
>>> index 1e3dc374bfde..d6dbf67acbb5 100644
>>> --- a/net/batman-adv/bat_v_ogm.c
>>> +++ b/net/batman-adv/bat_v_ogm.c
>>> @@ -200,7 +200,7 @@ static void batadv_v_ogm_send(struct work_struct
>>> *work)
>>>   type = "unknown";
>>>   }
>>> -batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "OGM2 from
>>> ourselve on %s surpressed: %s\n",
>>> +batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "OGM2 from
>>> ourself on %s surpressed: %s\n",
>>
>> I think proper English is "ourselves"...
> 
>Sorry. "ourself" also seems valid. :-)
> 
>> [...]
> 
> MBR, Sergei

Yep, my bad grammar fail. I'll resend with some extra fixes.

Re: [PATCH][net-next] qtnfmac: fix uninitialized return code in ret

[Colin Ian King]

On 21/06/17 14:54, Sergey Matyukevich wrote:
>> The return value ret is unitialized and garbage is being returned
>> for the three different error conditions when setting up the PCIe
>> BARs. Fix this by initializing ret to  -ENOMEM to indicate that
>> the BARs failed to be setup correctly.
>>
>> Detected by CoverityScan, CID#1437563 ("Unitialized scalar variable")
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c 
>> b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c
>> index f93b27f3a236..7fc4f0d6a9ad 100644
>> --- a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c
>> +++ b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c
>> @@ -247,7 +247,7 @@ static void qtnf_pcie_free_shm_ipc(struct 
>> qtnf_pcie_bus_priv *priv)
>>
>>  static int qtnf_pcie_init_memory(struct qtnf_pcie_bus_priv *priv)
>>  {
>> -   int ret;
>> +   int ret = -ENOMEM;
>>
>> priv->sysctl_bar = qtnf_map_bar(priv, QTN_SYSCTL_BAR);
>> if (IS_ERR_OR_NULL(priv->sysctl_bar)) {
>> --
> 
> Thanks !
> 
> Reviewed-by: Sergey Matyukevich <sergey.matyukevich...@quantenna.com>
> 
> By the way, could you please use the recepient list suggested by
> get_maintainer.pl script from the kernel scripts directory.

Yep, I did.

> 
> Regards,
> Sergey
> 

Re: [PATCH][netdev-next] net: hns: make guid hns_dsaf_acpi_dsm_guid static

[Colin Ian King]

On 13/06/17 20:24, Andy Shevchenko wrote:
> On Tue, Jun 13, 2017 at 8:56 PM, David Miller <da...@davemloft.net> wrote:
>> From: Colin King <colin.k...@canonical.com>
>> Date: Tue, 13 Jun 2017 14:03:21 +0100
>>
>>> From: Colin Ian King <colin.k...@canonical.com>
>>>
>>> The guid hns_dsaf_acpi_dsm_guid does not need to be in global
>>> scope, so make it static.
>>>
>>> Cleans up sparse warning:
>>> "symbol 'hns_dsaf_acpi_dsm_guid' was not declared. Should it be static?"
>>>
>>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>>
>> That symbol doesn't even exist in th net-next tree.
> 
> It looks like the patch is done against UUID tree.
> Cc'ed Christoph.
> 
Sorry, I messed up on determining the original tree it came from in
linux-next when I looked at the output from get_maintainer.pl

Colin

re: phy: cpcap-usb: Add CPCAP PMIC USB support

[Colin Ian King]

Hi Tony,

While running static analysis on linux-next, CoverityScan picked up a
NULL pointer deference on ddata->pins when calling pinctrl_lookup_state:

466ddata->pins = devm_pinctrl_get(ddata->dev);

   1. Condition IS_ERR(ddata->pins), taking true branch.

467if (IS_ERR(ddata->pins)) {
468dev_info(ddata->dev, "default pins not configured:
%ld\n",
469 PTR_ERR(ddata->pins));

   2. assign_zero: Assigning: ddata->pins = NULL.

470ddata->pins = NULL;
471}
472

   CID 1440453 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)3.
var_deref_model: Passing null pointer ddata->pins to
pinctrl_lookup_state, which dereferences it. [show details]

473ddata->pins_ulpi = pinctrl_lookup_state(ddata->pins, "ulpi");


I suspect the IS_ERROR() check should return with some error return
rather than continuing.

Colin.

Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value

[Colin Ian King]

On 03/06/17 16:55, Andy Shevchenko wrote:
> On Fri, Jun 2, 2017 at 5:58 PM, Colin King  wrote:
>> The current comparison of entry < 0 will never be true since entry is an
>> unsigned integer. Cast entry to an int to ensure -ve error return values
>> from the call to jumbo_frm are correctly being caught.
> 
>> if (unlikely(is_jumbo) && likely(priv->synopsys_id <
>>  DWMAC_CORE_4_00)) {
>> entry = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion);
>> -   if (unlikely(entry < 0))
>> +   if (unlikely((int)entry < 0))
> 
> It feels like a hiding some other issue.
> 

The alternative is:

int rc = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion);
if (unlikely(rc < 0))
goto dma_map_err;

entry = rc;

however, that is effectively the same. The cast I'm using is a well used
idiom in the kernel, it used in almost a hundred similar cases.

git grep "< 0" | grep "(int)" | wc -l
95

Colin

Re: [PATCH] net: phy: marvell: make two functions static

[Colin Ian King]

On 02/06/17 15:00, Andrew Lunn wrote:
> On Fri, Jun 02, 2017 at 12:14:24PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> functions m88e1510_get_temp_critical and m88e1510_get_temp_alarm can be
>> made static as they not need to be in global scope.
>>
>> Cleans up sparse warnings:
>>  "symbol 'm88e1510_get_temp_alarm' was not declared. Should it be static?"
>>  "symbol 'm88e1510_get_temp_critical' was not declared. Should it be 
>>   static?"
> 
> Hi Colin
> 
> What about m88e1510_get_temp_alarm()? That should also have a static.

Ah, good point, I somehow missed that one. I'll send a V2

Colin
> 
> Thanks
>  Andrew
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] ethernet: aquantia: remove redundant checks on error status

[Colin Ian King]

On 11/05/17 19:16, David Miller wrote:
> From: Colin King <colin.k...@canonical.com>
> Date: Thu, 11 May 2017 18:29:29 +0100
> 
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> The error status err is initialized as zero and then being checked
>> several times to see if it is less than zero even when it has not
>> been updated. Since these checks are redundant we can remove these
>> as well as err and the error exit label err_exit.
>>
>> Detected by CoverityScan, CID#1398313 and CID#1398306 ("Logically
>> dead code")
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> 
> Please enhance your commit message to also explain that the functions
> being called around these checks all return void, to make it clear
> that this isn't an issue of the return values not being checked.
> 
> Thanks.
> 
Good idea. Will do.

Re: [PATCH] wlcore: fix spelling mistake in wl1271_warning 'iligal' -> 'illegal'

[Colin Ian King]

On 03/04/17 10:20, Joe Perches wrote:
> On Mon, 2017-04-03 at 10:15 +0100, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> trivial fix to spelling mistake in wl1271_warning error message
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  drivers/net/wireless/ti/wlcore/debugfs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ti/wlcore/debugfs.c 
>> b/drivers/net/wireless/ti/wlcore/debugfs.c
>> index 58e148d7bc7b..416080adc181 100644
>> --- a/drivers/net/wireless/ti/wlcore/debugfs.c
>> +++ b/drivers/net/wireless/ti/wlcore/debugfs.c
>> @@ -1249,7 +1249,7 @@ static ssize_t fw_logger_write(struct file *file,
>>  }
>>  
>>  if (wl->conf.fwlog.output == 0) {
>> -wl1271_warning("iligal opperation - fw logger disabled by 
>> default, please change mode via wlconf");
>> +wl1271_warning("illegal opperation - fw logger disabled by 
>> default, please change mode via wlconf");
> 
> Hi Colin.
> 
> When you do these would you please fix all the typos on the
> same line?  opperation -> operation

Sure, I stupidly missed that.

> 
> Also, generally, invalid is a better word choice than illegal
> for these situations.

Good point.
> 
> Thanks and cheers, Joe
> 

re: rxrpc: Fix deadlock between call creation and sendmsg/recvmsg

[Colin Ian King]

I think the following part of the patch is problematic:

call = rxrpc_find_call_by_user_ID(rx, user_call_ID);
if (!call) {
+   ret = -EBADSLT;
if (cmd != RXRPC_CMD_SEND_DATA)
-   return -EBADSLT;
+   goto error_release_sock;
+   ret = -EBUSY;

At this point call is null, so the following code is performing a null
pointer dereference on call when accessing call->state.

Detected by CoverityScan CID#1414316 ("Dereference after null check")

+   if (call->state == RXRPC_CALL_UNINITIALISED ||
+   call->state == RXRPC_CALL_CLIENT_AWAIT_CONN ||
+   call->state == RXRPC_CALL_SERVER_PREALLOC ||
+   call->state == RXRPC_CALL_SERVER_SECURING ||
+   call->state == RXRPC_CALL_SERVER_ACCEPTING)
+   goto error_release_sock;
call = rxrpc_new_client_call_for_sendmsg(rx, msg,
user_call_ID,

Re: [PATCH][V2] rtlwifi: rtl8192de: ix spelling mistake: "althougth" -> "though"

[Colin Ian King]

On 26/02/17 18:52, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> trivial fix to spelling mistake in RT_TRACE message
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c 
> b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
> index de98d88..dcb5d83 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c
> @@ -812,7 +812,7 @@ bool rtl92d_phy_config_rf_with_headerfile(struct 
> ieee80211_hw *hw,
>* pathA or mac1 has to set phy0 pathA */
>   if ((content == radiob_txt) && (rfpath == RF90_PATH_A)) {
>   RT_TRACE(rtlpriv, COMP_INIT, DBG_LOUD,
> -  " ===> althougth Path A, we load radiob.txt\n");
> +  " ===> though Path A, we load radiob.txt\n");
>   radioa_arraylen = radiob_arraylen;
>   radioa_array_table = radiob_array_table;
>   }
> 
OOPS, ignore that.

re: sched: check negative err value to safe one level of indent

[Colin Ian King]

Jiro,

A recent static analysis run with CoverityScan identified a potential
change in functionality with your recent commit "sched: check negative
err value to safe one level of indent" that landed in linux-next.

The original path for case RTM_DELTFILTER would always goto errout, but
your commit seems to goto errout only if err is non-zero zero and the
err == 0 path falls through to the RTM_GETTFILTER case.  I'm not sure if
this is an intentional change in behaviour.  From what I can make out,
there is a missing goto errout before the fall-through to RTM_GETTFILTER.

Colin

re: sfc: process RX event inner checksum flags

[Colin Ian King]

Hi there,


not sure if this is a bug, or intentional, but CoverityScan picked up a
mismatch in arguments when calling efx_ef10_handle_rx_event_error() with
commit "sfc: process RX event inner checksum flags" that landed in
linux-next:

  CID 1402067 (#1 of 1): Arguments in wrong order
(SWAPPED_ARGUMENTS)swapped_arguments: The positions of arguments in the
call to efx_ef10_handle_rx_event_errors do not match the ordering of the
parameters:

rx_l3_class is passed to rx_encap_hdr
rx_l4_class is passed to rx_l3_class
rx_encap_hdr is passed to rx_l4_class


The function in question has the prototype:

static u16 efx_ef10_handle_rx_event_errors(struct efx_channel *channel,
  unsigned int n_packets,
  unsigned int rx_encap_hdr,
  unsigned int rx_l3_class,
  unsigned int rx_l4_class,
  const efx_qword_t *event)

...where as it it being called using:

flags |= efx_ef10_handle_rx_event_errors(channel, n_packets,
rx_l3_class, rx_l4_class, rx_encap_hdr, event);

Is this a bug or intentional?

Colin

Re: mlxsw: spectrum: Introduce ACL core with simple TCAM implementation

[Colin Ian King]

Hi Jiri,

mlxsw_sp_acl_tcam_chunk_create has an issue picked up by static analysis:

 816struct mlxsw_sp_acl_tcam_chunk *chunk;
 817int err;
 818
CID 1400029 (#1 of 1): Operands don't affect result
(CONSTANT_EXPRESSION_RESULT)result_independent_of_operands: priority ==
18446744073709551615UL is always false regardless of the values of its
operands. This occurs as the logical operand of "if".

 819if (priority == MLXSW_SP_ACL_TCAM_CATCHALL_PRIO)
 820return ERR_PTR(-EINVAL);
 821

priority is an unsigned int, where as MLXSW_SP_ACL_TCAM_CATCHALL_PRIO is
(-1UL), so the unsigned comparison to a signed long (-1UL) will never be
true. So I think this needs some reworking, especially as
MLXSW_SP_ACL_TCAM_CATCHALL_PRIO priority is being used as an unsigned
long priority in some places, and priority is an unsigned int in other
places of the code.

Colin

Re: [PATCH][net-next] net: bridge: remove redundant check to see if err is set

[Colin Ian King]

..and one more thing, in net/bridge/br_netlink_tunnel.c,
__get_num_vlan_tunnel_infos, are the args to vlan_tunnel_id_isrange()
the wrong way around? I'm not 100% sure, but I'd thought I'd flag it up.


if (v_start) {
if ((v_end->vid - v->vid) > 0 &&
vlan_tunnel_id_isrange(v_end, v) > 0)
num_tinfos += 2;
else
num_tinfos += 1;
}



On 07/02/17 11:30, Nikolay Aleksandrov wrote:
> On 07/02/17 11:56, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> The error check on err is redundant as it is being checked
>> previously each time it has been updated.  Remove this redundant
>> check.
>>
>> Detected with CoverityScan, CID#140030("Logically dead code")
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  net/bridge/br_netlink.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index fc5d885..cdc4e2a 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -612,9 +612,6 @@ static int br_afspec(struct net_bridge *br,
>>  return err;
>>  break;
>>  }
>> -
>> -if (err)
>> -return err;
>>  }
>>  
>>  return err;
>>
> 
> Actually that code can be reduced further, I'll follow up with a patch later.
> 
> Reviewed-by: Nikolay Aleksandrov <niko...@cumulusnetworks.com>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: bridge: per vlan dst_metadata netlink support

[Colin Ian King]

Hi,

Static analysis with CoverityScan on net/bridge/br_netlink_tunnel.c has
picked up and issue where tb[] is being referenced but contains
uninitialized data.

222 int br_parse_vlan_tunnel_info(struct nlattr *attr,
223   struct vtunnel_info *tinfo)
224 {

1. var_decl: Declaring variable tb without initializer.

225 struct nlattr *tb[IFLA_BRIDGE_VLAN_TUNNEL_MAX + 1];
226 u32 tun_id;
227 u16 vid, flags = 0;
228 int err;
229
230 memset(tinfo, 0, sizeof(*tinfo));
231

CID 1400055 (#1 of 1): Uninitialized pointer read (UNINIT)2.
uninit_use: Using uninitialized value tb[IFLA_BRIDGE_VLAN_TUNNEL_ID].

232 if (!tb[IFLA_BRIDGE_VLAN_TUNNEL_ID] ||
233 !tb[IFLA_BRIDGE_VLAN_TUNNEL_VID])
234 return -EINVAL;

Colin

Re: [patch net-next] sfc: a couple off by one bugs

[Colin Ian King]

On 01/02/17 13:24, Edward Cree wrote:
> On 01/02/17 08:50, Dan Carpenter wrote:
>> These checks are off by one.  These are just sanity checks and we don't
>> ever pass invalid values for "encap_type" so it's harmless.
>>
>> Fixes: 9b4108012517 ("sfc: insert catch-all filters for encapsulated 
>> traffic")
>> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> One of these was already fixed by Colin Ian King in
> e9904990e8e70a51574e6ec6b872f3c705ec75f0 ("sfc: fix an off-by-one compare on 
> an
> array size").

Ugh, I missed one of those.

> For the other one:
> Acked-by: Edward Cree <ec...@solarflare.com>
>> diff --git a/drivers/net/ethernet/sfc/ef10.c 
>> b/drivers/net/ethernet/sfc/ef10.c
>> index 8bec9383d754..dec0c8083ff3 100644
>> --- a/drivers/net/ethernet/sfc/ef10.c
>> +++ b/drivers/net/ethernet/sfc/ef10.c
>> @@ -5080,7 +5080,7 @@ static int efx_ef10_filter_insert_def(struct efx_nic 
>> *efx,
>>
>>   /* quick bounds check (BCAST result impossible) */
>>   BUILD_BUG_ON(EFX_EF10_BCAST != 0);
>> - if (encap_type > ARRAY_SIZE(map) || map[encap_type] == 0) {
>> + if (encap_type >= ARRAY_SIZE(map) || map[encap_type] == 0) {
>>   WARN_ON(1);
>>   return -EINVAL;
>>   }
>> @@ -5134,7 +5134,7 @@ static int efx_ef10_filter_insert_def(struct efx_nic 
>> *efx,
>>
>>   /* quick bounds check (BCAST result impossible) */
>>   BUILD_BUG_ON(EFX_EF10_BCAST != 0);
>> - if (encap_type > ARRAY_SIZE(map) || map[encap_type] == 0) {
>> + if (encap_type >= ARRAY_SIZE(map) || map[encap_type] == 0) {
>>   WARN_ON(1);
>>   return -EINVAL;
>>   }
> 
> 
> The information contained in this message is confidential and is intended for 
> the addressee(s) only. If you have received this message in error, please 
> notify the sender immediately and delete the message. Unless you are an 
> addressee (or authorized to receive for an addressee), you may not use, copy 
> or disclose to anyone this message or any information contained in this 
> message. The unauthorized use, disclosure, copying or alteration of this 
> message is strictly prohibited.
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch net-next] sctp: fix some debug output

[Colin Ian King]

On 24/01/17 09:51, Dan Carpenter wrote:
> On Tue, Jan 24, 2017 at 07:14:11AM -0200, Marcelo Ricardo Leitner wrote:
>> On Tue, Jan 24, 2017 at 12:05:40PM +0300, Dan Carpenter wrote:
>>> We added SCTP_EVENT_TIMEOUT_RECONF but we didn't update this array so
>>> it causes an off-by-one read overflow.
>>>
>>> Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
>>> Signed-off-by: Dan Carpenter 
>>
>> Weird, seems your patch is missing the --- marker here.
>> Colin (Cc'ed) is supposed to post a v3 of his patch containing this fix.
>>
> 
> Colin, if you wanted, you could CC kernel-janitors with static checker
> fixes.  There are several of us on that list.

OK, thanks for the heads-up on that Dan.

Colin

> 
> regards,
> dan carpenter
> 

Re: [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl

[Colin Ian King]

On 20/01/17 13:10, Marcelo Ricardo Leitner wrote:
> On Fri, Jan 20, 2017 at 01:01:57PM +, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> The comparison on the timeout can lead to an array overrun
>> read on sctp_timer_tbl because of an off-by-one error. Fix
>> this by using < instead of <= and also compare to the array
>> size rather than SCTP_EVENT_TIMEOUT_MAX.
>>
>> Fixes CoverityScan CID#1397639 ("Out-of-bounds read")
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  net/sctp/debug.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/sctp/debug.c b/net/sctp/debug.c
>> index 95d7b15..e371a0d 100644
>> --- a/net/sctp/debug.c
>> +++ b/net/sctp/debug.c
>> @@ -166,7 +166,7 @@ static const char *const sctp_timer_tbl[] = {
>>  /* Lookup timer debug name. */
>>  const char *sctp_tname(const sctp_subtype_t id)
>>  {
>> -if (id.timeout <= SCTP_EVENT_TIMEOUT_MAX)
>> +if (id.timeout < ARRAY_SIZE(sctp_timer_tbl))
>>  return sctp_timer_tbl[id.timeout];
> 
> The issue exists but this is not the right fix.
> Issue was introduced by 7b9438de0cd4 ("sctp: add stream reconf timer")
> as it introduced a new timer but didn't add the timer name here, so the
> fix should (also) include:
> 
> diff --git a/net/sctp/debug.c b/net/sctp/debug.c
> index 95d7b15dad21..c5f4ed5242ac 100644
> --- a/net/sctp/debug.c
> +++ b/net/sctp/debug.c
> @@ -159,6 +159,7 @@ static const char *const sctp_timer_tbl[] = {
>   "TIMEOUT_T4_RTO",
>   "TIMEOUT_T5_SHUTDOWN_GUARD",
>   "TIMEOUT_HEARTBEAT",
> + "TIMEOUT_RECONF",
>   "TIMEOUT_SACK",
>   "TIMEOUT_AUTOCLOSE",
>  };

Ah, OK, I can add that timeout into the the table, but perhaps it's
still prudent to check the index against the table size as well.

> 
> Thanks,
> Marcelo
> 
>>  return "unknown_timer";
>>  }
>> -- 
>> 2.10.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH] flow disector: check if arp_eth is null rather than arp

[Colin Ian King]

On 13/01/17 18:24, Eric Dumazet wrote:
> On Fri, 2017-01-13 at 13:34 +, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> arp is being checked instead of arp_eth to see if the call to
>> __skb_header_pointer failed. Fix this by checking arp_eth is
>> null instead of arp.
>>
>> CoverityScan CID#1396428 ("Logically dead code") on 2nd
>> arp comparison (which should be arp_eth instead).
>>
>> Fixes: commit 55733350e5e8b70c5 ("flow disector: ARP support")
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  net/core/flow_dissector.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index e3dffc7..fec48e9 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -409,7 +409,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>> sizeof(_arp_eth), data,
>> hlen - sizeof(_arp),
>> &_arp_eth);
>> -if (!arp)
>> +if (!arp_eth)
>>  goto out_bad;
>>  
>>  if (dissector_uses_key(flow_dissector,
> 
> It looks that we try very hard to add critical bugs in flow dissector.
> 
> This is embarrassing really.
> 
> I am questioning if the __skb_header_pointer() is correct
> 
> Why using hlen - sizeof(_arp) ?
> 
>arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp),
>   sizeof(_arp_eth), data,
>   hlen - sizeof(_arp),
>   &_arp_eth);
> 

Yep, the sizeof maybe dubious too, I overlooked that one; if somebody
can clarify that then I'll send a V2 if it needs fixing up too.

Colin

Re: [PATCH] [media] gp8psk: fix spelling mistake: "firmare" -> "firmware"

[Colin Ian King]

On 29/12/16 21:23, VDR User wrote:
>> -   err("firmare chunk size bigger than 64 bytes.");
>> +   err("firmware chunk size bigger than 64 bytes.");
> 
> Yup.
> 
>> -"HW don't support CMAC encrypiton, use software 
>> CMAC encrypiton\n");
>> +"HW don't support CMAC encryption, use software 
>> CMAC encryption\n");
> 
> Should be: "HW doesn't support CMAC encryption, use software CMAC
> encryption\n");
> 
Very true, I was so focused on the spelling I overlooked the grammar.
I'll re-send with that fixed.

Colin

Re: [PATCH][V2] qed: fix memory leak of a qed_spq_entry on error failure paths

[Colin Ian King]

On 21/12/16 13:29, Mintz, Yuval wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> A qed_spq_entry entry is allocated by qed_sp_init_request but is not kfree'd
>> if an error occurs, causing a memory leak. Fix this by returning the 
>> previously
>> allocated spq entry and also setting *pp_ent to NULL to be safe.
>>
>> Thanks to Yuval Mintz for suggestions on how to improve my original fix.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> 
> We've given it a more thorough look, and apparently this isn't the correct 
> fix.
> So I'll start by saying sorry for making you send this V2 needlessly.
> 
> It boils down to the fact there are two kinds of SPQ entries -
> Those originating from the 'free_pool' and those from the 'unlimited_pending'.
> Only those originating from the free_pool should be returned
> using the qed_spq_return_entry(), as only those actually point to a valid
> dma-mapped memory where FW expects to find the entries;
> Returning the other kind would lead to assertions later,
> as driver would post a ramrod to FW which actually points to address 0.
> 
> Looking at the error flows, it seems possible this isn't the only faulty
> error flow in the SPQ. I suggest you'd drop this and we'll take it from
> here [although if you really have the urge to continue - please do].
> 
> Thanks,
> Yuval
> 
> 
Sure, lets drop my fixes, I'm out of time on this for 2016 anyhow.

Colin

Re: [PATCH] kcm: fix spelling mistake in Kconfig, "connectons"

[Colin Ian King]

On 13/12/16 17:30, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Trivial fix to spelling mistake "connectons" to "connections" in
> Kconfig text.
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  net/kcm/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/kcm/Kconfig b/net/kcm/Kconfig
> index 87fca36..23b01e1 100644
> --- a/net/kcm/Kconfig
> +++ b/net/kcm/Kconfig
> @@ -7,5 +7,5 @@ config AF_KCM
>   ---help---
> KCM (Kernel Connection Multiplexor) sockets provide a method
> for multiplexing messages of a message based application
> -   protocol over kernel connectons (e.g. TCP connections).
> +   protocol over kernel connections (e.g. TCP connections).
>  
> 
Oops, ignore that, I was working on the wrong tree.

Re: [PATCH] cxgb4: fix memory leak on txq_info

[Colin Ian King]

On 25/11/16 21:10, David Miller wrote:
> From: Colin King <colin.k...@canonical.com>
> Date: Wed, 23 Nov 2016 11:02:44 +
> 
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> Currently if txq_info->uldtxq cannot be allocated then
>> txq_info->txq is being kfree'd (which is redundant because it
>> is NULL) instead of txq_info. Fix this by instead kfree'ing
>> txq_info.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> 
> Applied, but Colin you _really_ need to start properly marking your
> networking patch submissions by indicating in the subject which
> tree your change is for.  In this case I figured out it was
> net-next, but you must say this explicitly in the Subject line
> via "Subject: [PATCH net-next] ..."
> 
> Thanks.
> 
Understood, will do next time, apologies for that.

Colin

re: amd-xgbe: Add support for MDIO attached PHYs

[Colin Ian King]

Hi,

Commit:

amd-xgbe: Add support for MDIO attached PHYs

Use the phylib support in the kernel to communicate with and control an
MDIO attached PHY. Use the hardware's MDIO communication mechanism to
communicate with the PHY.


+static int xgbe_clr_gpio(struct xgbe_prv_data *pdata, unsigned int gpio)
+{
+   unsigned int reg;
+
+   if (gpio > 16)
+   return -EINVAL;

is gpio in the range 0..15?

if (gpio > 15)
return -EINVAL;

+
+   reg = XGMAC_IOREAD(pdata, MAC_GPIOSR);
+
+   reg &= ~(1 << (gpio + 16));

if gpio is 16, we get 1 << 32 which I believe is undefined behaviour.

+   XGMAC_IOWRITE(pdata, MAC_GPIOSR, reg);
+
+   return 0;
+}


Same applies for function xgbe_clr_gpio().

Colin

Re: [PATCH][V2] mlxsw: spectrum: remove redundant check if err is zero

[Colin Ian King]

On 24/09/16 22:08, Ido Schimmel wrote:
> On Sat, Sep 24, 2016 at 06:03:38PM -0700, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> There is an earlier check and return if err is non-zero, so
>> the check to see if it is zero is redundant in every iteration
>> of the loop and hence the check can be removed.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> 
> The subject and commit message are wrong. I think you copy-pasted them
> from an earlier patch.

Oops, jetlagged cut-n-paste error. I'll resend
> 
>> ---
>>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c | 10 +-
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c 
>> b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c
>> index 2a61617..1073673 100644
>> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c
>> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c
>> @@ -117,11 +117,11 @@ static int validate_filter(struct net_device *dev,
>>  return 0;
>>  }
>>  
>> -static unsigned int get_filter_steerq(struct net_device *dev,
>> -  struct ch_filter_specification *fs)
>> +static int get_filter_steerq(struct net_device *dev,
>> + struct ch_filter_specification *fs)
>>  {
>>  struct adapter *adapter = netdev2adap(dev);
>> -unsigned int iq;
>> +int iq;
>>  
>>  /* If the user has requested steering matching Ingress Packets
>>   * to a specific Queue Set, we need to make sure it's in range
>> @@ -443,10 +443,10 @@ int __cxgb4_set_filter(struct net_device *dev, int 
>> filter_id,
>> struct filter_ctx *ctx)
>>  {
>>  struct adapter *adapter = netdev2adap(dev);
>> -unsigned int max_fidx, fidx, iq;
>> +unsigned int max_fidx, fidx;
>>  struct filter_entry *f;
>>  u32 iconf;
>> -int ret;
>> +int iq, ret;
>>  
>>  max_fidx = adapter->tids.nftids;
>>  if (filter_id != (max_fidx + adapter->tids.nsftids - 1) &&
>> -- 
>> 2.9.3
>>

Re: [PATCH] mwifiex: fix memory leak on regd when chan is zero

[Colin Ian King]

On 15/09/16 18:10, Kalle Valo wrote:
> Colin King <colin.k...@canonical.com> writes:
> 
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> When chan is zero mwifiex_create_custom_regdomain does not kfree
>> regd and we have a memory leak. Fix this by freeing regd before
>> the return.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c 
>> b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>> index 3344a26..15a91f3 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>> @@ -1049,8 +1049,10 @@ mwifiex_create_custom_regdomain(struct 
>> mwifiex_private *priv,
>>  enum nl80211_band band;
>>  
>>  chan = *buf++;
>> -if (!chan)
>> +if (!chan) {
>> +kfree(regd);
>>  return NULL;
>> +}
> 
> Bob sent a similar fix and he also did more:
> 
> mwifiex: fix error handling in mwifiex_create_custom_regdomain
> 
> https://patchwork.kernel.org/patch/9331337/
> 
Ah, sorry for the duplication noise.

Colin

Re: [PATCH] ath10k: fix memory leak on caldata on error exit path

[Colin Ian King]

On 02/09/16 16:45, Valo, Kalle wrote:
> Colin King <colin.k...@canonical.com> writes:
> 
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> caldata is not being free'd on the error exit path, causing
>> a memory leak. kfree it to fix the leak.
>>
>> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/pci.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c 
>> b/drivers/net/wireless/ath/ath10k/pci.c
>> index 9a22c47..886337c 100644
>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>> @@ -2725,6 +2725,7 @@ static int ath10k_pci_hif_fetch_cal_eeprom(struct 
>> ath10k *ar, void **data,
>>  return 0;
>>  
>>  err_free:
>> +kfree(caldata);
>>  kfree(data);
>>  
>>  return -EINVAL;
> 
> I don't think we should free data at all:
> 
> static int ath10k_download_cal_eeprom(struct ath10k *ar)
> {
>   size_t data_len;
>   void *data = NULL;
>   int ret;
> 
>   ret = ath10k_hif_fetch_cal_eeprom(ar, , _len);
> 
> Instead we should free only caldata, right?
> 
Yep, good catch, I'll send V2 later.

Colin

NACK: [PATCH] nfp: check idx is -ENOSPC before using it is an index

[Colin Ian King]

Ignore this, got some other fix included by mistake. Will resend.

On 11/07/16 16:46, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> idx can be returned as -ENOSPC, so we should check for this first
> before using it as an index into nn->vxlan_usecnt[] to avoid an
> out of bounds array offset read.
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 2 +-
>  drivers/nfc/fdp/fdp.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c 
> b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> index 1e74b91..88678c1 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> @@ -2578,7 +2578,7 @@ static void nfp_net_del_vxlan_port(struct net_device 
> *netdev,
>   return;
>  
>   idx = nfp_net_find_vxlan_idx(nn, ti->port);
> - if (!nn->vxlan_usecnt[idx] || idx == -ENOSPC)
> + if (idx == -ENOSPC || !nn->vxlan_usecnt[idx])
>   return;
>  
>   if (!--nn->vxlan_usecnt[idx])
> diff --git a/drivers/nfc/fdp/fdp.c b/drivers/nfc/fdp/fdp.c
> index e44a7a2..d93d314 100644
> --- a/drivers/nfc/fdp/fdp.c
> +++ b/drivers/nfc/fdp/fdp.c
> @@ -345,7 +345,7 @@ static void fdp_nci_release_firmware(struct nci_dev *ndev)
>  
>   if (info->ram_patch) {
>   release_firmware(info->ram_patch);
> - info->otp_patch = NULL;
> + info->ram_patch = NULL;
>   }
>  }
>  
> 

Re: [PATCH] net/ethoc: fix null dereference on error exit path

[Colin Ian King]

On 22/05/16 20:42, Max Filippov wrote:
> Hi Colin,
> 
> On Sun, May 22, 2016 at 08:08:18PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> priv is assigned to NULL however all the error exit paths to label 'free'
>> dereference priv, causing a null pointer dereference.
>>
>> Examination of the code shows that all error exits via the 'free'
>> label path occur before priv is assigned to netdev_priv(netdev), hence
>> there is no need to call clk_disable_unprepare and so the location of
>> the label should be moved to free_netdev statement to avoid this null
>> dereference on priv.
> 
> This description is a bit inaccurate. Indeed all 'goto free' above the
> 'priv = netdev_priv(netdev);' need to skip 'if (priv->clk)' check, but
> there are two more 'goto free' below that line, and they look correct
> now, but after this patch they'll leave the clock enabled.
> 
Oops, I'll resend a corrected fix tomorrow

Re: [PATCH] rtlwifi: pass struct rtl_stats by reference as it is more efficient

[Colin Ian King]

On 22/02/16 06:51, Alexander Stein wrote:
> On Saturday 20 February 2016 22:10:27, Colin King wrote:
>> From: Colin Ian King <colin.k...@canonical.com>
>>
>> passing rtl_stats by value is inefficient; the structure is over 300
>> bytes in size and generally just one field (packet_report_type)
>> is being accessed, so the pass by value is a relatively large overhead.
>> This change just affects just the rx_command_packet calls.
> 
> Why not using a const pointer?

const struct rtl_stats *const status?

> 
> Best regards,
> Alexander
> 

Re: rhashtable: Fix walker list corruption

[Colin Ian King]

On 16/12/15 08:45, Herbert Xu wrote:
> On Fri, Oct 09, 2015 at 11:32:23AM +0100, Colin Ian King wrote:
>>
>> I'm hitting a null ptr deference bug when running 2 or more instances of
>> the attached reproducer program.  I've bisected this down to the
>> following commit:
>>
>> commit ba7c95ea3870fe7b847466d39a049ab6f156aa2c
>> Author: Herbert Xu <herb...@gondor.apana.org.au>
>> Date:   Tue Mar 24 09:53:17 2015 +1100
>>
>> rhashtable: Fix sleeping inside RCU critical section in walk_stop
>>
>>
>> Without this commit, the attached reproducer runs fine for hours. With
>> the commit, I can oops a 4 core (8 thread) Intel i7-6700 Sharkbay SDP in
>> a few seconds.
> 
> Thanks Colin.  This commit was indeed bogus, as we end up using
> two different locks for the one list.

I've given this a good soak test and it fixes the issue. Thanks Herbert!

Colin

> 
> ---8<---
> The commit ba7c95ea3870fe7b847466d39a049ab6f156aa2c ("rhashtable:
> Fix sleeping inside RCU critical section in walk_stop") introduced
> a new spinlock for the walker list.  However, it did not convert
> all existing users of the list over to the new spin lock.  Some
> continued to use the old mutext for this purpose.  This obviously
> led to corruption of the list.
> 
> The fix is to use the spin lock everywhere where we touch the list.
> 
> This also allows us to do rcu_rad_lock before we take the lock in
> rhashtable_walk_start.  With the old mutex this would've deadlocked
> but it's safe with the new spin lock.
> 
> Fixes: ba7c95ea3870 ("rhashtable: Fix sleeping inside RCU...")
> Reported-by: Colin Ian King <colin.k...@canonical.com>
> Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au>
> 
> diff --git a/lib/rhashtable.c b/lib/rhashtable.c
> index 1c624db..ed7ba47 100644
> --- a/lib/rhashtable.c
> +++ b/lib/rhashtable.c
> @@ -519,10 +519,10 @@ int rhashtable_walk_init(struct rhashtable *ht, struct 
> rhashtable_iter *iter)
>   if (!iter->walker)
>   return -ENOMEM;
>  
> - mutex_lock(>mutex);
> + spin_lock(>lock);
>   iter->walker->tbl = rht_dereference(ht->tbl, ht);
>   list_add(>walker->list, >walker->tbl->walkers);
> - mutex_unlock(>mutex);
> + spin_unlock(>lock);
>  
>   return 0;
>  }
> @@ -536,10 +536,10 @@ EXPORT_SYMBOL_GPL(rhashtable_walk_init);
>   */
>  void rhashtable_walk_exit(struct rhashtable_iter *iter)
>  {
> - mutex_lock(>ht->mutex);
> + spin_lock(>ht->lock);
>   if (iter->walker->tbl)
>   list_del(>walker->list);
> - mutex_unlock(>ht->mutex);
> + spin_unlock(>ht->lock);
>   kfree(iter->walker);
>  }
>  EXPORT_SYMBOL_GPL(rhashtable_walk_exit);
> @@ -563,14 +563,12 @@ int rhashtable_walk_start(struct rhashtable_iter *iter)
>  {
>   struct rhashtable *ht = iter->ht;
>  
> - mutex_lock(>mutex);
> + rcu_read_lock();
>  
> + spin_lock(>lock);
>   if (iter->walker->tbl)
>   list_del(>walker->list);
> -
> - rcu_read_lock();
> -
> - mutex_unlock(>mutex);
> + spin_unlock(>lock);
>  
>   if (!iter->walker->tbl) {
>   iter->walker->tbl = rht_dereference_rcu(ht->tbl, ht);
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

rhashtable concurrent writers issue, 4.2+

[Colin Ian King]

Hi,

After testing with commit 7def0f952eccdd0edb3c504f4dab35ee0d3aba1f
("lib: fix data race in rhashtable_rehash_one") on the current 4.4-rc4
kernel, I believe I am still seeing race conditions that seem to bite
concurrent readers.

The reproducer is found in my stress-ng system stress tool:

git://kernel.ubuntu.com/cking/stress-ng.git

one needs libattr1-dev, libkeyutils-dev to build this.

run the concurrent procfs stressor on 2 CPUs:

./stress-ng --procfs 2

..sometimes this hangs w/o a kernel stack dump, but sometimes it oopses
and one can see that we get a concurrent reader failure.

Colin
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html