[dpdk-dev] Define statement with UB prevents compilation using UBSAN

[dpdk-dev] Define statement with UB prevents compilation using UBSAN

[Owen Hilyard]

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

Re: [dpdk-dev] Define statement with UB prevents compilation using UBSAN

[Stephen Hemminger]

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)?

Re: [dpdk-dev] Define statement with UB prevents compilation using UBSAN

[Morten Brørup]

> 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! :-)

Re: [dpdk-dev] Define statement with UB prevents compilation using UBSAN

[Aaron Conole]

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.

Re: [dpdk-dev] Define statement with UB prevents compilation using UBSAN

[Owen Hilyard]

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.
>
>