DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] Define statement with UB prevents compilation using UBSAN
@ 2021-06-10 20:51 Owen Hilyard
  2021-06-10 21:36 ` Stephen Hemminger
  0 siblings, 1 reply; 5+ messages in thread
From: Owen Hilyard @ 2021-06-10 20:51 UTC (permalink / raw)
  To: Aaron Conole, David Marchand; +Cc: dev

Hello,

While starting work on adding UBSAN to the community lab CI, I found that
DPDK does not compile in its default configuration with UBSAN enabled,
failing with the error message:

../drivers/net/bnx2x/bnx2x.c: In function ‘bnx2x_check_blocks_with_parity3’:
../drivers/net/bnx2x/bnx2x.c:3363:4: error: case label does not reduce to
an integer constant
 3363 |    case AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY:
      |    ^~~~

Working backward to the define
statement, AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY is defined as

#define AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY (0x1 << 31)

While this does set the most significant bit of the integer, it also seems
to make UBSAN unable to properly track its type. Replacing this with

#define AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY (-2147483648)

or

#define AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY
(0b10000000000000000000000000000000)

Allows compilation to finish on x86_64, but substantially breaks the style
of the rest of the file. There are a few other places where this undefined
behavior happens:

./drivers/baseband/acc100/rte_acc100_pmd.c:4490: value = (1 << 31) + (23 <<
8) + (1 << 6) + 7;
./drivers/net/bnxt/tf_core/tf_shadow_tcam.c:60:#define
TF_SHADOW_TCAM_HB_HANDLE_IS_VALID(hndl) (((hndl) & (1 << 31)) != 0)
./drivers/net/bnxt/tf_core/tf_shadow_tcam.c:61:#define
TF_SHADOW_TCAM_HB_HANDLE_CREATE(idx, be) ((1 << 31) | \
./drivers/net/bnxt/tf_core/tf_shadow_tbl.c:60:#define
TF_SHADOW_HB_HANDLE_IS_VALID(hndl) (((hndl) & (1 << 31)) != 0)
./drivers/net/bnxt/tf_core/tf_shadow_tbl.c:61:#define
TF_SHADOW_HB_HANDLE_CREATE(idx, be) ((1 << 31) | \
./drivers/net/e1000/base/e1000_regs.h:197:#define E1000_TQAVCC_QUEUE_MODE
(1 << 31) /* SP vs. SR Tx mode */
./drivers/net/e1000/base/e1000_regs.h:607:#define E1000_ETQF_QUEUE_ENABLE
(1 << 31)
./drivers/net/igc/base/igc_regs.h:201:#define IGC_TQAVCC_QUEUE_MODE (1 <<
31) /* SP vs. SR Tx mode */
./drivers/net/igc/base/igc_82575.h:248:#define IGC_ETQF_QUEUE_ENABLE (1 <<
31)
./drivers/net/igc/base/igc_82575.h:271:#define IGC_DTXSWC_VMDQ_LOOPBACK_EN
(1 << 31)  /* global VF LB enable */
./drivers/net/bnx2x/ecore_hsi.h:2597: #define TRIGGER_MDUMP_ONCE      (1 <<
31)
./drivers/net/bnx2x/ecore_hsi.h:3904:#define IGU_REGULAR_BCLEANUP (0x1 <<
31)
./drivers/net/bnx2x/ecore_reg.h:4366:#define
AEU_INPUTS_ATTN_BITS_CCM_HW_INTERRUPT (0x1 << 31)
./drivers/net/bnx2x/ecore_reg.h:4391:#define
AEU_INPUTS_ATTN_BITS_PBCLIENT_HW_INTERRUPT (0x1 << 31)
./drivers/net/ixgbe/base/ixgbe_type.h:4295:#define
IXGBE_KRM_LINK_CTRL_1_TETH_AN_RESTART (1 << 31)
./drivers/net/ixgbe/base/ixgbe_type.h:4325:#define
IXGBE_KRM_TX_COEFF_CTRL_1_OVRRD_EN (1 << 31)
./drivers/crypto/qat/qat_sym_pmd.h:24:#define QAT_SYM_CAP_VALID (1 << 31)
./drivers/bus/fslmc/portal/dpaa2_hw_pvt.h:301:#define
DPAA2_SET_FLE_FIN(fle) ((fle)->fin_bpid_offset |= 1 << 31)
./drivers/raw/ioat/ioat_spec.h:329:#define CMDSTATUS_ACTIVE_MASK (1 << 31)
./drivers/common/mlx5/windows/mlx5_win_defs.h:114: IBV_RX_HASH_INNER = (1
<< 31)

Aside from normal CI, I don't have a good way to verify that a patch to fix
this wouldn't have hard to detect effects. As far as I know, some of these
drivers don't have DTS run on them in the course of CI, so I'm somewhat
wary of trying to make a change to them. Until this is fixed, deploying
UBSAN in any real capacity won't be useful since every patch will fail.
What do the two of you think about steps moving forward on this problem?

Owen Hilyard

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] Define statement with UB prevents compilation using UBSAN
  2021-06-10 20:51 [dpdk-dev] Define statement with UB prevents compilation using UBSAN Owen Hilyard
@ 2021-06-10 21:36 ` Stephen Hemminger
  2021-06-11  9:19   ` Morten Brørup
  2021-06-11 14:34   ` Aaron Conole
  0 siblings, 2 replies; 5+ messages in thread
From: Stephen Hemminger @ 2021-06-10 21:36 UTC (permalink / raw)
  To: Owen Hilyard; +Cc: Aaron Conole, David Marchand, dev

On Thu, 10 Jun 2021 16:51:37 -0400
Owen Hilyard <ohilyard@iol.unh.edu> wrote:

> Working backward to the define
> statement, AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY is defined as
> 
> #define AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY (0x1 << 31)

Why not (1u << 31)?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] Define statement with UB prevents compilation using UBSAN
  2021-06-10 21:36 ` Stephen Hemminger
@ 2021-06-11  9:19   ` Morten Brørup
  2021-06-11 14:34   ` Aaron Conole
  1 sibling, 0 replies; 5+ messages in thread
From: Morten Brørup @ 2021-06-11  9:19 UTC (permalink / raw)
  To: Owen Hilyard; +Cc: Aaron Conole, David Marchand, dev, Stephen Hemminger

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Thursday, 10 June 2021 23.36
> 
> On Thu, 10 Jun 2021 16:51:37 -0400
> Owen Hilyard <ohilyard@iol.unh.edu> wrote:
> 
> > Working backward to the define
> > statement, AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY is defined
> as
> >
> > #define AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY (0x1 << 31)
> 
> Why not (1u << 31)?

Yes, this is better. You want the type of the defined constant to be uint32_t.

It can also be defined in hexadecimal form as (0x1u << 31), or (0b1u << 31) in binary form.

<rant>
On the CPUs/compilers supported by DPDK, the exact type of an unsigned integer is uint32_t. If the CPU/compiler used 64 bits for integers, you would need additional type casting to get it down from 64 to 32 bits.

If we were to be really pedantic, the other defined constants that fit in a signed integer, e.g. (1 << 30) should also be defined as (1u << 30) if they are supposed to be unsigned integers. However, not many people worry about signedness when writing code, especially if an unsigned value fits into a signed variable or constant.
</rant>

Thank you for your effort on this, Owen. Keep up the good work! :-)


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] Define statement with UB prevents compilation using UBSAN
  2021-06-10 21:36 ` Stephen Hemminger
  2021-06-11  9:19   ` Morten Brørup
@ 2021-06-11 14:34   ` Aaron Conole
  2021-06-11 18:15     ` Owen Hilyard
  1 sibling, 1 reply; 5+ messages in thread
From: Aaron Conole @ 2021-06-11 14:34 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Owen Hilyard, David Marchand, dev, Rasesh Mody, Shahed Shaikh

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Thu, 10 Jun 2021 16:51:37 -0400
> Owen Hilyard <ohilyard@iol.unh.edu> wrote:
>
>> Working backward to the define
>> statement, AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY is defined as
>> 
>> #define AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY (0x1 << 31)
>
> Why not (1u << 31)?

+1

CC'd the QLogic maintainers as well.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] Define statement with UB prevents compilation using UBSAN
  2021-06-11 14:34   ` Aaron Conole
@ 2021-06-11 18:15     ` Owen Hilyard
  0 siblings, 0 replies; 5+ messages in thread
From: Owen Hilyard @ 2021-06-11 18:15 UTC (permalink / raw)
  To: Aaron Conole
  Cc: Stephen Hemminger, David Marchand, dev, Rasesh Mody, Shahed Shaikh

Seeing the discussion so far, do we want to change the single definition to
be (0b1u << 31) so it works, or should we make this change in a wider scope
(file, directory, project-wide). If we do make the change in a wider scope,
should we only change instances where there is UB (1 << 31) or should we
change all of the bitflags and similar constructs to uint32_t? If we change
a lot, it may require special testing since I don't think every driver is
tested on a regular basis, and making a change like this in a wide-reaching
fashion has the potential to break a lot of things.

On Fri, Jun 11, 2021 at 10:34 AM Aaron Conole <aconole@redhat.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> writes:
>
> > On Thu, 10 Jun 2021 16:51:37 -0400
> > Owen Hilyard <ohilyard@iol.unh.edu> wrote:
> >
> >> Working backward to the define
> >> statement, AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY is defined as
> >>
> >> #define AEU_INPUTS_ATTN_BITS_MCP_LATCHED_SCPAD_PARITY (0x1 << 31)
> >
> > Why not (1u << 31)?
>
> +1
>
> CC'd the QLogic maintainers as well.
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-06-11 18:16 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10 20:51 [dpdk-dev] Define statement with UB prevents compilation using UBSAN Owen Hilyard
2021-06-10 21:36 ` Stephen Hemminger
2021-06-11  9:19   ` Morten Brørup
2021-06-11 14:34   ` Aaron Conole
2021-06-11 18:15     ` Owen Hilyard

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git