From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 42824C433F5 for ; Fri, 11 Feb 2022 19:50:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236065AbiBKTuF (ORCPT ); Fri, 11 Feb 2022 14:50:05 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:59972 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242073AbiBKTuD (ORCPT ); Fri, 11 Feb 2022 14:50:03 -0500 Received: from mx0b-00082601.pphosted.com (mx0b-00082601.pphosted.com [67.231.153.30]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 26E592A5 for ; Fri, 11 Feb 2022 11:50:02 -0800 (PST) Received: from pps.filterd (m0148460.ppops.net [127.0.0.1]) by mx0a-00082601.pphosted.com (8.16.1.2/8.16.1.2) with ESMTP id 21BF6hCR004246 for ; Fri, 11 Feb 2022 11:50:01 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fb.com; h=from : to : cc : subject : date : message-id : in-reply-to : references : content-type : content-transfer-encoding : mime-version; s=facebook; bh=vKucdm/8VEDUN7ktf9EBcIimdtWR0LqQrEgRQo8i1Wk=; b=Bhw9gRZqee7exEi6oGi1S/EVTb4UqIwyGII51hCzSRV85ZFiQww+QiyXMVNaCK2QjBcw 2CQPP/qUrfoWdJY2tRkimx4hIuPdS+4nIUJQDS47OQhL7MYqETWyFyy4KUU22RF2geqa UmActIDhsHda4WvWrp1EywWn66f7ctYUbII= Received: from mail.thefacebook.com ([163.114.132.120]) by mx0a-00082601.pphosted.com (PPS) with ESMTPS id 3e58y9yucb-20 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Fri, 11 Feb 2022 11:50:01 -0800 Received: from twshared0983.05.ash8.facebook.com (2620:10d:c085:208::11) by mail.thefacebook.com (2620:10d:c085:11d::5) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Fri, 11 Feb 2022 11:49:57 -0800 Received: by devbig309.ftw3.facebook.com (Postfix, from userid 128203) id B7A4C6430129; Fri, 11 Feb 2022 11:49:53 -0800 (PST) From: Yonghong Song To: CC: Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , , Kumar Kartikeya Dwivedi Subject: [PATCH bpf v4 2/2] bpf: fix a bpf_timer initialization issue Date: Fri, 11 Feb 2022 11:49:53 -0800 Message-ID: <20220211194953.3142152-1-yhs@fb.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20220211194943.3141324-1-yhs@fb.com> References: <20220211194943.3141324-1-yhs@fb.com> Content-Type: text/plain; charset="UTF-8" X-FB-Internal: Safe X-Proofpoint-GUID: EIFrxq08M-5px3DFyapzDCk_xh1eha12 X-Proofpoint-ORIG-GUID: EIFrxq08M-5px3DFyapzDCk_xh1eha12 Content-Transfer-Encoding: quoted-printable X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.62.513 definitions=2022-02-11_05,2022-02-11_01,2021-12-02_01 X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 clxscore=1015 mlxlogscore=428 impostorscore=0 mlxscore=0 suspectscore=0 lowpriorityscore=0 bulkscore=0 spamscore=0 phishscore=0 priorityscore=1501 malwarescore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000 definitions=main-2202110104 X-FB-Internal: deliver Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org The patch in [1] intends to fix a bpf_timer related issue, but the fix caused existing 'timer' selftest to fail with hang or some random errors. After some debug, I found an issue with check_and_init_map_value() in the hashtab.c. More specifically, in hashtab.c, we have code l_new =3D bpf_map_kmalloc_node(&htab->map, ...) check_and_init_map_value(&htab->map, l_new...) Note that bpf_map_kmalloc_node() does not do initialization so l_new contains random value. The function check_and_init_map_value() intends to zero the bpf_spin_lock and bpf_timer if they exist in the map. But I found bpf_spin_lock is zero'ed but bpf_timer is not zero'ed. With [1], later copy_map_value() skips copying of bpf_spin_lock and bpf_timer. The non-zero bpf_timer caused random failures for 'timer' selftest. Without [1], for both bpf_spin_lock and bpf_timer case, bpf_timer will be zero'ed, so 'timer' self test is okay. For check_and_init_map_value(), why bpf_spin_lock is zero'ed properly while bpf_timer not. In bpf uapi header, we have struct bpf_spin_lock { __u32 val; }; struct bpf_timer { __u64 :64; __u64 :64; } __attribute__((aligned(8))); The initialization code: *(struct bpf_spin_lock *)(dst + map->spin_lock_off) =3D (struct bpf_spin_lock){}; *(struct bpf_timer *)(dst + map->timer_off) =3D (struct bpf_timer){}; It appears the compiler has no obligation to initialize anonymous fields. For example, let us use clang with bpf target as below: $ cat t.c struct bpf_timer { unsigned long long :64; }; struct bpf_timer2 { unsigned long long a; }; void test(struct bpf_timer *t) { *t =3D (struct bpf_timer){}; } void test2(struct bpf_timer2 *t) { *t =3D (struct bpf_timer2){}; } $ clang -target bpf -O2 -c -g t.c $ llvm-objdump -d t.o ... 0000000000000000 : 0: 95 00 00 00 00 00 00 00 exit 0000000000000008 : 1: b7 02 00 00 00 00 00 00 r2 =3D 0 2: 7b 21 00 00 00 00 00 00 *(u64 *)(r1 + 0) =3D r2 3: 95 00 00 00 00 00 00 00 exit gcc11.2 does not have the above issue. But from INTERNATIONAL STANDARD =C2=A9ISO/IEC ISO/IEC 9899:201x Programming languages =E2=80=94 C http://www.open-std.org/Jtc1/sc22/wg14/www/docs/n1547.pdf page 157: Except where explicitly stated otherwise, for the purposes of this subclause unnamed members of objects of structure and union type do not participate in initialization. Unnamed members of structure objects have indeterminate value even after initialization. To fix the problem, let use memset for bpf_timer case in check_and_init_map_value(). For consistency, memset is also used for bpf_spin_lock case. [1] https://lore.kernel.org/bpf/20220209070324.1093182-2-memxor@gmail.com/ Fixes: 68134668c17f3 ("bpf: Add map side support for bpf timers.") Signed-off-by: Yonghong Song --- include/linux/bpf.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index fa517ae604ad..1a4c73742a1f 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -209,11 +209,9 @@ static inline bool map_value_has_timer(const struct bp= f_map *map) static inline void check_and_init_map_value(struct bpf_map *map, void *dst) { if (unlikely(map_value_has_spin_lock(map))) - *(struct bpf_spin_lock *)(dst + map->spin_lock_off) =3D - (struct bpf_spin_lock){}; + memset(dst + map->spin_lock_off, 0, sizeof(struct bpf_spin_lock)); if (unlikely(map_value_has_timer(map))) - *(struct bpf_timer *)(dst + map->timer_off) =3D - (struct bpf_timer){}; + memset(dst + map->timer_off, 0, sizeof(struct bpf_timer)); } =20 /* copy everything but bpf_spin_lock and bpf_timer. There could be one of = each. */ --=20 2.30.2