[bug report] net: ipa: reduce arguments to ipa_table_init_add()

[bug report] net: ipa: reduce arguments to ipa_table_init_add()

[Dan Carpenter]

Hello Alex Elder,

This is a semi-automatic email about new static checker warnings.

The patch 5cb76899fb47: "net: ipa: reduce arguments to 
ipa_table_init_add()" from Nov 2, 2022, leads to the following Smatch 
complaint:

    drivers/net/ipa/ipa_table.c:423 ipa_table_init_add()
    error: we previously assumed 'hash_mem' could be null (see line 414)

drivers/net/ipa/ipa_table.c
   413			count = mem->size / sizeof(__le64);
   414			hash_count = hash_mem && hash_mem->size / sizeof(__le64);
                                     ^^^^^^^^
The patch adds checks for NULL.

   415		}
   416		size = count * sizeof(__le64);
   417		hash_size = hash_count * sizeof(__le64);
   418	
   419		addr = ipa_table_addr(ipa, filter, count);
   420		hash_addr = ipa_table_addr(ipa, filter, hash_count);
   421	
   422		ipa_cmd_table_init_add(trans, opcode, size, mem->offset, addr,
   423				       hash_size, hash_mem->offset, hash_addr);
                                                  ^^^^^^^^^^^^^^^^
Unchecked dereference.

   424		if (!filter)
   425			return;

regards,
dan carpenter

Re: [bug report] net: ipa: reduce arguments to ipa_table_init_add()

[Alex Elder]

On 11/15/22 07:03, Dan Carpenter wrote:
> Hello Alex Elder,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 5cb76899fb47: "net: ipa: reduce arguments to
> ipa_table_init_add()" from Nov 2, 2022, leads to the following Smatch
> complaint:
> 
>      drivers/net/ipa/ipa_table.c:423 ipa_table_init_add()
>      error: we previously assumed 'hash_mem' could be null (see line 414)

This is a nice catch.  It turns out that the issue pointed out
is not a problem in practice.  The 8 possible memory regions
that could be used are (currently) always defined, so both mem
and hash_mem will be non-null.  Nevertheless Smatch identifies
a real problem with the code in this function, and I will fix
that.

IN ADDITION...  This report forced me to examine this patch once
more, and I now see a different bug from what was reported,
which I'll point out below.

I intend to fix both problems with one patch.

> 
> drivers/net/ipa/ipa_table.c
>     413			count = mem->size / sizeof(__le64);
>     414			hash_count = hash_mem && hash_mem->size / sizeof(__le64);

Line 414 is wrong.  It should be:
     hash_count = hash_mem ? hash_mem->size / sizeof(__le64) : 0;

As it is now, the count will be 1 or 0, and it should be the
number of entries that fit in the region (or 0 if hash_mem is
a null pointer).

I did test this during development and verified the counts were
correct, HOWEVER I must have tested this on a system that did
not support hashed tables (IPA v4.2).  I'm very happy for this
Smatch report.

					-Alex

>                                       ^^^^^^^^
> The patch adds checks for NULL.
> 
>     415		}
>     416		size = count * sizeof(__le64);
>     417		hash_size = hash_count * sizeof(__le64);
>     418	
>     419		addr = ipa_table_addr(ipa, filter, count);
>     420		hash_addr = ipa_table_addr(ipa, filter, hash_count);
>     421	
>     422		ipa_cmd_table_init_add(trans, opcode, size, mem->offset, addr,
>     423				       hash_size, hash_mem->offset, hash_addr);
>                                                    ^^^^^^^^^^^^^^^^
> Unchecked dereference.
> 
>     424		if (!filter)
>     425			return;
> 
> regards,
> dan carpenter

Re: [bug report] net: ipa: reduce arguments to ipa_table_init_add()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

On Tue, Nov 15, 2022 at 11:00:49AM -0600, Alex Elder wrote:
> > drivers/net/ipa/ipa_table.c
> >     413			count = mem->size / sizeof(__le64);
> >     414			hash_count = hash_mem && hash_mem->size / sizeof(__le64);
> 
> Line 414 is wrong.  It should be:
>     hash_count = hash_mem ? hash_mem->size / sizeof(__le64) : 0;
> 

Heh.  It really feels like this line should have generated a checker
warning as well.  I've created two versions.  The first complains when
ever there is a divide used as a condition:

	if (a / b) {

The other complains when it's part of a logical && or ||.

	if (a && a / b) {

drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition: 'hash_mem->size / 8'
drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition (logical): 'hash_mem->size / 8'

I'll test them out tonight and see if either gives useful results.

regards,
dan carpenter


[-- Attachment #2: check_divide_condition.c --]
[-- Type: text/x-csrc, Size: 1604 bytes --]

/*
 * Copyright (C) 2022 Dan Carpenter.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_extra.h"

static int my_id;

static void match_condition1(struct expression *expr)
{
	char *name;

	if (expr->type != EXPR_BINOP || expr->op != '/')
		return;

	name = expr_to_str(expr);
	sm_warning("divide condition: '%s'", name);
	free_string(name);
}

static void match_condition2(struct expression *expr)
{
	struct expression *parent;
	char *name;

	if (expr->type != EXPR_BINOP || expr->op != '/')
		return;

	parent = expr_get_parent_expr(expr);
	while (parent && parent->type == EXPR_PREOP && parent->op == '(')
		parent = expr_get_parent_expr(parent);

	if (!parent || parent->type != EXPR_LOGICAL)
		return;

	name = expr_to_str(expr);
	sm_warning("divide condition (logical): '%s'", name);
	free_string(name);
}

void check_divide_condition(int id)
{
	my_id = id;

	add_hook(&match_condition1, CONDITION_HOOK);
	add_hook(&match_condition2, CONDITION_HOOK);

}

Re: [bug report] net: ipa: reduce arguments to ipa_table_init_add()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

On Thu, Nov 17, 2022 at 07:47:27AM +0300, Dan Carpenter wrote:
> Heh.  It really feels like this line should have generated a checker
> warning as well.  I've created two versions.  The first complains when
> ever there is a divide used as a condition:
> 
> 	if (a / b) {
> 
> The other complains when it's part of a logical && or ||.
> 
> 	if (a && a / b) {
> 
> drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition: 'hash_mem->size / 8'
> drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition (logical): 'hash_mem->size / 8'
> 
> I'll test them out tonight and see if either gives useful results.

I modified the test to ignore macros.  Otherwise we get warnings where
macros are trying to avoid divide by zero bugs.  There was no advantage
in treating logicals as special so I dropped that.

The results are kind of mind bending.  I think maybe people sometimes
accidentally write "if (a / b) {" meaning does it divide cleanly?  When
that should be written as: "if ((a % b) == 0) {".

Anyway, attached.

regards,
dan carpenter

[-- Attachment #2: err-list --]
[-- Type: text/plain, Size: 2871 bytes --]

drivers/nvdimm/claim.c:287 nsio_rw_bytes() warn: divide condition: 'cleared / 512'
drivers/nvdimm/bus.c:210 nvdimm_account_cleared_poison() warn: divide condition: 'cleared / 512'
drivers/rtc/rtc-m48t59.c:135 m48t59_rtc_set_time() warn: divide condition: 'year / 100'
drivers/gpu/drm/i915/gvt/vgpu.c:124 intel_gvt_init_vgpu_types() warn: divide condition: 'low_avail / conf->low_mm'
drivers/regulator/aat2870-regulator.c:142 aat2870_get_regulator() warn: divide condition: '(id - 1) / 2'
drivers/leds/leds-bcm6328.c:116 bcm6328_led_mode() warn: divide condition: 'shift / 16'
drivers/leds/leds-bcm6328.c:357 bcm6328_led() warn: divide condition: 'shift / 16'
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:22554 wlc_phy_rssi_cal_nphy_rev3() warn: divide condition: 'result_idx / 2'
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c:22926 wlc_phy_rssi_cal_nphy_rev2() warn: divide condition: 'result_idx / 2'
drivers/net/wireless/broadcom/b43/phy_n.c:2202 b43_nphy_rev3_rssi_cal() warn: divide condition: 'i / 2'
drivers/net/wireless/broadcom/b43/phy_n.c:2403 b43_nphy_rev2_rssi_cal() warn: divide condition: 'i / 2'
drivers/net/wireless/intersil/hostap/hostap_plx.c:232 hfa384x_from_bap() warn: divide condition: 'len / 2'
drivers/net/wireless/intersil/hostap/hostap_plx.c:251 hfa384x_to_bap() warn: divide condition: 'len / 2'
drivers/net/wireless/intersil/hostap/hostap_cs.c:164 hfa384x_from_bap() warn: divide condition: 'len / 2'
drivers/net/wireless/intersil/hostap/hostap_cs.c:183 hfa384x_to_bap() warn: divide condition: 'len / 2'
drivers/net/ethernet/intel/iavf/iavf_txrx.c:300 iavf_clean_tx_irq() warn: divide condition: 'j / 4'
drivers/net/ethernet/intel/i40e/i40e_txrx_common.h:77 i40e_arm_wb() warn: divide condition: 'j / 4'
drivers/net/ethernet/intel/i40e/i40e_txrx_common.h:77 i40e_arm_wb() warn: divide condition: 'j / 4'
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:759 fm10k_uc_vlan_unsync() warn: divide condition: 'vid / 4096'
drivers/net/ethernet/intel/fm10k/fm10k_netdev.c:779 fm10k_mc_vlan_unsync() warn: divide condition: 'vid / 4096'
drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition: 'hash_mem->size / 8'
drivers/gpio/gpio-exar.c:52 exar_offset_to_sel_addr() warn: divide condition: 'pin / 8'
drivers/gpio/gpio-exar.c:62 exar_offset_to_lvl_addr() warn: divide condition: 'pin / 8'
drivers/ata/ahci_imx.c:411 __sata_ahci_read_temperature() warn: divide condition: 'm2 / 1000'
fs/hfsplus/bitmap.c:46 hfsplus_block_allocate() warn: divide condition: '(size ^ offset) / (((1) << 12) * 8)'
fs/hfsplus/bitmap.c:88 hfsplus_block_allocate() warn: divide condition: '(size ^ offset) / (((1) << 12) * 8)'
fs/freevxfs/vxfs_bmap.c:62 vxfs_bmap_ext4() warn: divide condition: 'bn / (indsize * indsize * bsize / 4)'
sound/soc/codecs/rt274.c:881 rt274_set_bclk_ratio() warn: divide condition: 'ratio / 50'

Re: [bug report] net: ipa: reduce arguments to ipa_table_init_add()

[Alex Elder]

On 11/18/22 03:50, Dan Carpenter wrote:
> On Thu, Nov 17, 2022 at 07:47:27AM +0300, Dan Carpenter wrote:
>> Heh.  It really feels like this line should have generated a checker
>> warning as well.  I've created two versions.  The first complains when
>> ever there is a divide used as a condition:
>>
>> 	if (a / b) {
>>
>> The other complains when it's part of a logical && or ||.
>>
>> 	if (a && a / b) {
>>
>> drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition: 'hash_mem->size / 8'
>> drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition (logical): 'hash_mem->size / 8'
>>
>> I'll test them out tonight and see if either gives useful results.
> 
> I modified the test to ignore macros.  Otherwise we get warnings where
> macros are trying to avoid divide by zero bugs.  There was no advantage
> in treating logicals as special so I dropped that.
> 
> The results are kind of mind bending.  I think maybe people sometimes
> accidentally write "if (a / b) {" meaning does it divide cleanly?  When
> that should be written as: "if ((a % b) == 0) {".

Interesting.  I looked at the first few.  I think the nvdimm ones
might be using "if (cleared / 512)" to mean "if (cleared >= 512",
and in that case, the >= might actually be more efficient than the
divide.  On the real-time clock one it looked like a similar usage.
Regardless, it's not a typical idiom so I don't think it's
straightforward to understand.

And you're right, I saw at least one example that looked like
it was using divide when modulo was intended.

It's not trivial to understand the intent where this occurs
either.  Maybe that's what you mean by "mind bending".

I'm glad to have helped identify this case.  I hope it leads
to a few things getting fixed (or improving).

					-Alex

> 
> Anyway, attached.
> 
> regards,
> dan carpenter

Re: [bug report] net: ipa: reduce arguments to ipa_table_init_add()

[Dan Carpenter]

On Sat, Nov 19, 2022 at 03:21:48AM -0600, Alex Elder wrote:
> On 11/18/22 03:50, Dan Carpenter wrote:
> > On Thu, Nov 17, 2022 at 07:47:27AM +0300, Dan Carpenter wrote:
> > > Heh.  It really feels like this line should have generated a checker
> > > warning as well.  I've created two versions.  The first complains when
> > > ever there is a divide used as a condition:
> > > 
> > > 	if (a / b) {
> > > 
> > > The other complains when it's part of a logical && or ||.
> > > 
> > > 	if (a && a / b) {
> > > 
> > > drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition: 'hash_mem->size / 8'
> > > drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition (logical): 'hash_mem->size / 8'
> > > 
> > > I'll test them out tonight and see if either gives useful results.
> > 
> > I modified the test to ignore macros.  Otherwise we get warnings where
> > macros are trying to avoid divide by zero bugs.  There was no advantage
> > in treating logicals as special so I dropped that.
> > 
> > The results are kind of mind bending.  I think maybe people sometimes
> > accidentally write "if (a / b) {" meaning does it divide cleanly?  When
> > that should be written as: "if ((a % b) == 0) {".
> 
> Interesting.  I looked at the first few.  I think the nvdimm ones
> might be using "if (cleared / 512)" to mean "if (cleared >= 512",
> and in that case, the >= might actually be more efficient than the
> divide.  On the real-time clock one it looked like a similar usage.
> Regardless, it's not a typical idiom so I don't think it's
> straightforward to understand.

Yeah.  I'm going to publish the check and the new warning message will
be:

drivers/nvdimm/claim.c:287 nsio_rw_bytes() warn: replace divide condition 'cleared / 512' with 'cleared >= 512'

regards,
dan carpenter

Re: [bug report] net: ipa: reduce arguments to ipa_table_init_add()

[Alex Elder]

On 11/16/22 22:47, Dan Carpenter wrote:
> On Tue, Nov 15, 2022 at 11:00:49AM -0600, Alex Elder wrote:
>>> drivers/net/ipa/ipa_table.c
>>>      413			count = mem->size / sizeof(__le64);
>>>      414			hash_count = hash_mem && hash_mem->size / sizeof(__le64);
>>
>> Line 414 is wrong.  It should be:
>>      hash_count = hash_mem ? hash_mem->size / sizeof(__le64) : 0;
>>
> 
> Heh.  It really feels like this line should have generated a checker
> warning as well.  I've created two versions.  The first complains when
> ever there is a divide used as a condition:

I agree, this is a good case to identify if you can.

> 
> 	if (a / b) {

That seems reasonable.  In a real "if" statement it is
along the lines of an assignment used as a condition,
where a second set of parentheses can be used to indicate
it's intentional.

> The other complains when it's part of a logical && or ||.
> 
> 	if (a && a / b) {

I don't know how Smatch works, but at the C parser level
it seems any divide used in a conditional context might
be worth paying attention to.

> 
> drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition: 'hash_mem->size / 8'
> drivers/net/ipa/ipa_table.c:414 ipa_table_init_add() warn: divide condition (logical): 'hash_mem->size / 8'
> 
> I'll test them out tonight and see if either gives useful results.

Sounds good.  Sorry I didn't respond a few days ago, I'm away
from home and am not keeping up with everything well.

					-Alex

> 
> regards,
> dan carpenter
>