From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 84B5943BE9; Mon, 26 Feb 2024 22:45:55 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 528B8402CC; Mon, 26 Feb 2024 22:45:55 +0100 (CET) Received: from mail-pf1-f171.google.com (mail-pf1-f171.google.com [209.85.210.171]) by mails.dpdk.org (Postfix) with ESMTP id 5910B40293 for ; Mon, 26 Feb 2024 22:45:53 +0100 (CET) Received: by mail-pf1-f171.google.com with SMTP id d2e1a72fcca58-6e4e7e2594cso1911856b3a.2 for ; Mon, 26 Feb 2024 13:45:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1708983952; x=1709588752; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=bqu7Pubw9VNVe8IsrCs4/onO432e2uZatCP6x2Ua6pA=; b=sO0norwBodnzvuS2aEJZB8jTXOuKHG6ItBhLeVW+F2au5PJ42C/okLjFsRKdfahENn 7WWi2oqO9UM1XM8ZID0vHT32WiGm3G5EfisXZPi4e1sYd7kdEaGLwOJV8lbke1fIz/eN VgmMHVr+WbkFQ5KwCSD4ZEOMoayUdQxMzhJQ1YXUrGd7Pd/GsukbhbuW6r36wBPUsrzZ jJjjSuFpIy0+zVKCFOyal8v5I3nljzkS3ejUmNF/5jO8fVQcfuAZTLrR4+Ak9QN/QOfQ OlPcXvnJxAMnWmWzE0y0n4941Rd6qGfqOBl490N8M3xqcFAUGyOxldSjYkN1co4AgtJC DKPw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708983952; x=1709588752; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=bqu7Pubw9VNVe8IsrCs4/onO432e2uZatCP6x2Ua6pA=; b=Sh8gId8bd6PIs9MvEUvFVkDreZBuw/BZx+7OBcrOVrJY7a6GVH7w/uRmRlohfSUOcq hIfnDzfgVgcDY5AO1mNcNKRxAfXcE96Dfv1bnVveaM5QdkGypa8F4d2eEwmTk0FJLRgv hZZlrRPKKKHWt1SNVPoUo5qdUTZPoBZP6SrAmgOzOlUa3FNuLcfcyYinA2f7RVjwOqVt 1iZ4IpAXx3/VVcOu2rxOanzMvdiRTPnDvom1tSts5Fv4bYr+7P7u2SsR2NNP96F4uWf3 SjF8E45zYeXBSjp1HooCrNalkIFIiKPPvGWBLogzDypJGjlk/uoEjjl9KrzAFtbrrXPu Ffuw== X-Forwarded-Encrypted: i=1; AJvYcCUFIeRTndA+tR7S6JpKrFDCzDHju8m50cpINH72EYwsQxXHw/7OB0BTCRX/L7SnyQKf0r/njSWGVypqbZ0= X-Gm-Message-State: AOJu0YymT9xO+Y7l5KzYN4aqgt3SBjYm5VS9sHljAzMqME+QhaVR+xZu 5D9TN3KdCQr1gN/b1aJPVpUcnVXBFysbHvpXaZMjv6syftaFkqvAZBSnNtHp+aQ= X-Google-Smtp-Source: AGHT+IFCVYw1BYEfdveZIw1Hw36EInMWUz2BLuHSzKkGm+kbRa0EL94BGmDktohez99u0JcfF0U0Cw== X-Received: by 2002:a05:6a20:2d08:b0:1a0:fcf5:c930 with SMTP id g8-20020a056a202d0800b001a0fcf5c930mr532054pzl.27.1708983952477; Mon, 26 Feb 2024 13:45:52 -0800 (PST) Received: from hermes.local (204-195-123-141.wavecable.com. [204.195.123.141]) by smtp.gmail.com with ESMTPSA id q21-20020a62e115000000b006e471c54861sm4474855pfh.210.2024.02.26.13.45.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Feb 2024 13:45:52 -0800 (PST) Date: Mon, 26 Feb 2024 13:45:49 -0800 From: Stephen Hemminger To: Morten =?UTF-8?B?QnLDuHJ1cA==?= Cc: julien_dpdk@jaube.fr, jerinjacobk@gmail.com, dev@dpdk.org, rmody@marvell.com, shshaikh@marvell.com, palok@marvell.com Subject: Re: [PATCH v9] net/bnx2x: fix warnings about rte_memcpy lengths Message-ID: <20240226134549.55138a24@hermes.local> In-Reply-To: <20240223140056.130844-1-mb@smartsharesystems.com> References: <20240223140056.130844-1-mb@smartsharesystems.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, 23 Feb 2024 15:00:56 +0100 Morten Br=C3=B8rup wrote: > Bugfix: The vlan in the bulletin does not contain a VLAN header, only the > VLAN ID, so only copy 2 byte, not 4. The target structure has padding > after the field, so copying 2 byte too many is effectively harmless. > There is no need to backport this patch. >=20 > Use RTE_PTR_ADD where copying arrays to the offset of a first field in a > structure holding multiple fields, to avoid compiler warnings with > decorated rte_memcpy. >=20 > Bugzilla ID: 1146 >=20 > Fixes: 540a211084a7695a1c7bc43068934c140d6989be ("bnx2x: driver core") > Cc: stephen@networkplumber.org > Cc: rmody@marvell.com > Cc: shshaikh@marvell.com > Cc: palok@marvell.com >=20 > Signed-off-by: Morten Br=C3=B8rup > Acked-by: Devendra Singh Rawat > --- > v9: > * Fix checkpatch warning about spaces. > v8: > * Use memcpy instead of rte_memcpy in slow path. (Stephen Hemminger) > v7: > * No changes. > v6: > * Add Fixes to patch description. > * Fix checkpatch warnings. > v5: > * No changes. > v4: > * Type casting did not fix the warnings, so use RTE_PTR_ADD instead. > v3: > * First patch in series. > --- > drivers/net/bnx2x/bnx2x_stats.c | 14 ++++++++------ > drivers/net/bnx2x/bnx2x_vfpf.c | 14 +++++++------- > 2 files changed, 15 insertions(+), 13 deletions(-) >=20 > diff --git a/drivers/net/bnx2x/bnx2x_stats.c b/drivers/net/bnx2x/bnx2x_st= ats.c > index c07b01510a..8105375d44 100644 > --- a/drivers/net/bnx2x/bnx2x_stats.c > +++ b/drivers/net/bnx2x/bnx2x_stats.c } > =20 > @@ -817,10 +817,10 @@ bnx2x_hw_stats_update(struct bnx2x_softc *sc) > etherstatspktsover1522octets); > } > =20 > - rte_memcpy(old, new, sizeof(struct nig_stats)); > + memcpy(old, new, sizeof(struct nig_stats)); This could just be structure assignment which is type safe. *new =3D *old; > =20 > - rte_memcpy(&(estats->rx_stat_ifhcinbadoctets_hi), &(pstats->mac_stx[= 1]), > - sizeof(struct mac_stx)); > + memcpy(RTE_PTR_ADD(estats, offsetof(struct bnx2x_eth_stats, rx_stat_ifh= cinbadoctets_hi)), > + &pstats->mac_stx[1], sizeof(struct mac_stx)); > estats->brb_drop_hi =3D pstats->brb_drop_hi; > estats->brb_drop_lo =3D pstats->brb_drop_lo; > =20 > @@ -1492,9 +1492,11 @@ bnx2x_stats_init(struct bnx2x_softc *sc) > REG_RD(sc, NIG_REG_STAT0_BRB_TRUNCATE + port*0x38); > if (!CHIP_IS_E3(sc)) { > REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT0 + port*0x50, > - &(sc->port.old_nig_stats.egress_mac_pkt0_lo), 2); > + RTE_PTR_ADD(&sc->port.old_nig_stats, > + offsetof(struct nig_stats, egress_mac_pkt0_lo)), 2); > REG_RD_DMAE(sc, NIG_REG_STAT0_EGRESS_MAC_PKT1 + port*0x50, > - &(sc->port.old_nig_stats.egress_mac_pkt1_lo), 2); > + RTE_PTR_ADD(&sc->port.old_nig_stats, > + offsetof(struct nig_stats, egress_mac_pkt1_lo)), 2); > } > =20 > /* function stats */ > diff --git a/drivers/net/bnx2x/bnx2x_vfpf.c b/drivers/net/bnx2x/bnx2x_vfp= f.c > index 63953c2979..5411df3a38 100644 > --- a/drivers/net/bnx2x/bnx2x_vfpf.c > +++ b/drivers/net/bnx2x/bnx2x_vfpf.c > @@ -52,9 +52,9 @@ bnx2x_check_bull(struct bnx2x_softc *sc) > =20 > /* check the mac address and VLAN and allocate memory if valid */ > if (valid_bitmap & (1 << MAC_ADDR_VALID) && memcmp(bull->mac, sc->old_b= ulletin.mac, ETH_ALEN)) > - rte_memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN); > + memcpy(&sc->link_params.mac_addr, bull->mac, ETH_ALEN); > if (valid_bitmap & (1 << VLAN_VALID)) > - rte_memcpy(&bull->vlan, &sc->old_bulletin.vlan, RTE_VLAN_HLEN); > + memcpy(&bull->vlan, &sc->old_bulletin.vlan, sizeof(bull->vlan)); > =20 > sc->old_bulletin =3D *bull; > =20 > @@ -569,7 +569,7 @@ bnx2x_vf_set_mac(struct bnx2x_softc *sc, int set) > =20 > bnx2x_check_bull(sc); > =20 > - rte_memcpy(query->filters[0].mac, sc->link_params.mac_addr, ETH_ALEN); > + memcpy(query->filters[0].mac, sc->link_params.mac_addr, ETH_ALEN); > =20 > bnx2x_add_tlv(sc, query, query->first_tlv.tl.length, > BNX2X_VF_TLV_LIST_END, > @@ -583,9 +583,9 @@ bnx2x_vf_set_mac(struct bnx2x_softc *sc, int set) > while (BNX2X_VF_STATUS_FAILURE =3D=3D reply->status && > bnx2x_check_bull(sc)) { > /* A new mac was configured by PF for us */ > - rte_memcpy(sc->link_params.mac_addr, sc->pf2vf_bulletin->mac, > + memcpy(sc->link_params.mac_addr, sc->pf2vf_bulletin->mac, > ETH_ALEN); > - rte_memcpy(query->filters[0].mac, sc->pf2vf_bulletin->mac, > + memcpy(query->filters[0].mac, sc->pf2vf_bulletin->mac, > ETH_ALEN); > =20 > rc =3D bnx2x_do_req4pf(sc, sc->vf2pf_mbox_mapping.paddr); > @@ -622,10 +622,10 @@ bnx2x_vf_config_rss(struct bnx2x_softc *sc, > BNX2X_VF_TLV_LIST_END, > sizeof(struct channel_list_end_tlv)); > =20 > - rte_memcpy(query->rss_key, params->rss_key, sizeof(params->rss_key)); > + memcpy(query->rss_key, params->rss_key, sizeof(params->rss_key)); > query->rss_key_size =3D T_ETH_RSS_KEY; > =20 > - rte_memcpy(query->ind_table, params->ind_table, T_ETH_INDIRECTION_TABLE= _SIZE); > + memcpy(query->ind_table, params->ind_table, T_ETH_INDIRECTION_TABLE_SIZ= E); > query->ind_table_size =3D T_ETH_INDIRECTION_TABLE_SIZE; > =20 > query->rss_result_mask =3D params->rss_result_mask; The driver is also using rte_memcpy for 2 byte values in bnx2x.c. Another issue is the driver is using one element array as a flexible array. A good static checker should catch this and report out of bounds access. union bnx2x_stats_show_data { uint32_t op; /* ioctl sub-command */ struct { uint32_t num; /* return number of stats */ uint32_t len; /* length of each string item */ } desc; /* variable length... */ char str[1]; /* holds names of desc.num stats, each desc.len in length = */ /* variable length... */ uint64_t stats[1]; /* holds all stats */ };