Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Song Liu <songliubraving@fb.com>
To:     Ian Rogers <irogers@google.com>
CC:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin Lau <kafai@fb.com>, Yonghong Song <yhs@fb.com>,
        Andrii Nakryiko <andriin@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@chromium.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] libbpf hashmap: Fix undefined behavior in hash_bits
Thread-Topic: [PATCH] libbpf hashmap: Fix undefined behavior in hash_bits
Thread-Index: AQHWrg3tjprFy4LddUi1ivGW9wO6rqmu2mWA
Date:   Thu, 29 Oct 2020 17:45:14 +0000
Message-ID: <497F86C5-BD00-4C38-BD87-C6EFB92D1088@fb.com>
References: <20201029160938.154084-1-irogers@google.com>
In-Reply-To: <20201029160938.154084-1-irogers@google.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <A721E4E0FAB0344EB41E1F7B834AB9A0@namprd15.prod.outlook.com>
X-MS-Exchange-CrossTenant-Network-Message-Id: 603015af-6abe-4dac-6608-08d87c326442
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Oct 2020 17:45:14.3358
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: /t3Ylasq+/YecZFl/SbPJt0CtjFRCDJAJQQnfcM7V4+xtNImH05FtRg//o9tvUVvdpk0LxrSgM29oLV2xWLcMg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB2726
X-OriginatorOrg: fb.com
Content-Transfer-Encoding: quoted-printable
X-Proofpoint-UnRewURL: 0 URL was un-rewritten
MIME-Version: 1.0
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.312,18.0.737
 definitions=2020-10-29_11:2020-10-29,2020-10-29 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 suspectscore=0
 bulkscore=0 phishscore=0 mlxscore=0 adultscore=0 mlxlogscore=999
 spamscore=0 malwarescore=0 lowpriorityscore=0 impostorscore=0
 clxscore=1011 priorityscore=1501 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2009150000 definitions=main-2010290123
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Oct 29, 2020, at 9:09 AM, Ian Rogers <irogers@google.com> wrote:
>=20
> If bits is 0, the case when the map is empty, then the >> is the size of
> the register which is undefined behavior - on x86 it is the same as a
> shift by 0. Fix by handling the 0 case explicitly when running with
> address sanitizer.
>=20
> A variant of this patch was posted previously as:
> https://lore.kernel.org/lkml/20200508063954.256593-1-irogers@google.com/
>=20
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/lib/bpf/hashmap.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>=20
> diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h
> index d9b385fe808c..27d0556527d3 100644
> --- a/tools/lib/bpf/hashmap.h
> +++ b/tools/lib/bpf/hashmap.h
> @@ -12,9 +12,23 @@
> #include <stddef.h>
> #include <limits.h>
>=20
> +#ifdef __has_feature
> +#define HAVE_FEATURE(f) __has_feature(f)
> +#else
> +#define HAVE_FEATURE(f) 0
> +#endif
> +
> static inline size_t hash_bits(size_t h, int bits)
> {
> 	/* shuffle bits and return requested number of upper bits */
> +#if defined(ADDRESS_SANITIZER) || HAVE_FEATURE(address_sanitizer)

I am not very familiar with these features. Is address sanitizer same
as undefined behavior sanitizer (mentioned in previous version)?

> +	/*
> +	 * If the requested bits =3D=3D 0 avoid undefined behavior from a
> +	 * greater-than bit width shift right (aka invalid-shift-exponent).
> +	 */
> +	if (bits =3D=3D 0)
> +		return -1;

Shall we return 0 or -1 (0xffffffff) here?=20

Also, we have HASHMAP_MIN_CAP_BITS =3D=3D 2. Shall we just make sure we
never feed bits =3D=3D 0 into hash_bits()?

Thanks,
Song


> +#endif
> #if (__SIZEOF_SIZE_T__ =3D=3D __SIZEOF_LONG_LONG__)
> 	/* LP64 case */
> 	return (h * 11400714819323198485llu) >> (__SIZEOF_LONG_LONG__ * 8 - bits=
);
> --=20
> 2.29.1.341.ge80a0c044ae-goog
>=20