Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a6e56396-f1f9-d939-cc25-201db242e3b8@fb.com>
Date:   Mon, 12 Sep 2022 17:02:22 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.13.0
Subject: Re: [PATCH v3 1/1] bpf: Ensure correct locking around vulnerable
 function find_vpid()
Content-Language: en-US
To:     Lee Jones <lee@kernel.org>
Cc:     linux-kernel@vger.kernel.org, Jiri Olsa <jolsa@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        John Fastabend <john.fastabend@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, bpf@vger.kernel.org
References: <20220912133855.1218900-1-lee@kernel.org>
 <243f7ca6-41e1-7522-a6fc-6a35d8104bb5@fb.com> <Yx9Mv4ltoNhuU0Om@google.com>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <Yx9Mv4ltoNhuU0Om@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TS9oYjQ3YnJaNHZtYnBFekVrd0JTVkZOdjA4TE0wR0MydFl4YlBFTW1CSmtl?=
 =?utf-8?B?RzFhekt1VjVqWGlrckVyYnZicVRTOWJ3ZmlQUWZjNmNja1BzSjBFcTJVK2Nx?=
 =?utf-8?B?ejFBYXhraS9RUDRsRDdaOXpUYTdleEp0SVhpalhhVm5KS2tweDMyK0QxcGNF?=
 =?utf-8?B?Y2JhOTNZbjYrNUhlV1hVWU43aFZYK3NJTlZ3SE11WGxkK2RWbm1vSUpJeGFV?=
 =?utf-8?B?Wk9Gd1ExNUR1elNDUStFZ2NkbkNwbTBlZVJINmdrcVY5UWNWOVlpK1FhT29n?=
 =?utf-8?B?bW1uaEFENldsUytXQ0F6YnlPVG5tZlY3dU1Zc1ZNL0tMbHB0VnZ6Y0VVUEwy?=
 =?utf-8?B?bkxNa0NWby9rRjJiYVUzVFNvaS9ZTFdNNzlxMlhyZGdzOXpTR2F4ekU3V0dH?=
 =?utf-8?B?R2NORG1RUVZ6YzRYdEg3RXp4UTVnTE0zSlFEOGhqR1BQYnFMZU0xRE54YWZV?=
 =?utf-8?B?eTNydmN5cDRHRDUyaFA5djhlV1N6NTBBKzZFQzRtemM0ZU1hMWlUYlc0dnV2?=
 =?utf-8?B?Z0J3a2p0N2lrNXY4UlBHZ2I4N0hHVG9WZ2RCN0Radzkxc0hueXRVb3RhSFMv?=
 =?utf-8?B?TkdKTFA3WDFYazdVRGN5c2dXSWJkZnZOVzRhYnFzUnVnaVlydmlBTnBDc0ZT?=
 =?utf-8?B?bTRLMnFTcVgxSlNNMEZtVStKQUh6UmRXZ0FyNUk2WW1Qb3VuNUVRdDFzZkNh?=
 =?utf-8?B?VG10Rm5qRllacXprcm1iNGV5WkU5cmh2OVl5VEp6aTNsL04xUmxNY1Qzck5I?=
 =?utf-8?B?emFVdUYyTk95Zk9NMmJUVHlBWSt5a1pvYUt4YXFRSTFVVEoxaTVIc041NVhq?=
 =?utf-8?B?NEVLbHN0YkxnM2Izb25EMlp4REcwc1hpczNXRlZSVHBUZlcySlBYS1c1KzB3?=
 =?utf-8?B?eDB6NDYydlREZDNQZWtrdDhYZmdxN1pTd1BIWjVucmVQUit3N2Q3YkFQM2Zz?=
 =?utf-8?B?RExDOTkwWDN0aWhlM09HNWx2dUlraThWT0gwZjVnQUV3ZDdYVnVnbEpBUysx?=
 =?utf-8?B?dDRDRzRiY0FBaEZ4VDBYWFJHMm96NSs0NXpReE9SQnVoUjloUW1IbWtIZEFa?=
 =?utf-8?B?YjA2YXQ5NXR4ZjduZmtPT01WQjl2UXVldC9tcFBkMjhaZWJYUmZTV3NNYUk0?=
 =?utf-8?B?ZlZDSndIOUp0S3d6YUFFV0NVbFU3RTM2TUNBeTFFanJIN01lOTRJZTl1TnN0?=
 =?utf-8?B?cGh1bE5FZWV3bUk5MW1KZHgzQWNTRmpTa3F5SEJyTldSbXpTRDQwTnZUcHgr?=
 =?utf-8?B?V0FrRmhsVlRIWDl1WStqZVFzQSt2cldabDNZRUxpZ21KM2FCTHRTdVU4aGVY?=
 =?utf-8?B?bmhpZWRxL1RXaUFvMzRoSFpZRVY2dU9JNUNqaGwxUDV6VVFZV21hUWlneGp1?=
 =?utf-8?B?MHNyZUlhS29GUHpLMzJuSEpXMnpPMDY0bE5FZ0VZZ0VhUklUN1VZS1Fkd3BW?=
 =?utf-8?B?T3k5UzFoMWl3OWFOUjE1alpUelg3MkNWNnB5L0cwZGFqUnVwTTZ0N2xOT0h4?=
 =?utf-8?B?WXBSaDMzbTZXNjU1U1ZNMzZ6a0hwMXIrMDQ0MnV4MUsxbVBQUWJSMGZQMWxI?=
 =?utf-8?B?aXhlc3oxQnRTYUUxUlFudS8wVGdaOGNSYlJNdVg4TXY1YXV4NWdjM1ZOazJ2?=
 =?utf-8?B?eFRtRFI5bDlrK3ZqRWRiZDNBeFZLV3oxc0p0MU8zZlFaQjcxdHJkTnFSRURT?=
 =?utf-8?B?VUxzcTQzaTdENkVuclNrMVZzMEhGcHlZY3ZZMVh1WHFMeDJwU2EwTGZFOEg5?=
 =?utf-8?B?NXlVODJNekpNQ2FqTVdKenF3M0tyRG50dUlSMXJyanlTOU5jd1M1OFZ1L0Zt?=
 =?utf-8?B?a21sSml5SUo0bUhjTnc0b25sWkNUanc1Z3NKVytnZlRaTm9PY3U4RUs4aEJC?=
 =?utf-8?B?SnJFMHNuWXpNRTNYekVsNWtNSzIxNUpjVVIyRjRxSTIxNnpFbmx5UnJLb3Z5?=
 =?utf-8?B?WDNrK2dNQ29IZ0JvR1BRVHIyakFRczRTWDlaUTQ3a3FMelc1cTVIcGxWNWY5?=
 =?utf-8?B?NjhpTHorVnFna2lwK0M2THpZVFBDZTFuK0xXa1pUVldwQWhXekZsU3Iyc3NR?=
 =?utf-8?B?eTNIQlJRdFNCeTgxZHpDenVESkRuWDJWa2FIZytWSkUySnFhL09ab1l5a0ZS?=
 =?utf-8?Q?7KUjvlhnNqGzcz8Sxy3DN5sue?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 34701259-7126-4eb8-84e6-08da94d8317b
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Sep 2022 16:02:29.1465
 (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: D5A8gdxWX/JSRaeH3grRPXP04EeLXLuxwE7G15DaxErKR1ueKggL4ZZ+YKbRTzyQ
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR15MB1801
X-Proofpoint-ORIG-GUID: 3YzhbF9NLv5ZaGLoRKsGJ1EOsuel8sl5
X-Proofpoint-GUID: 3YzhbF9NLv5ZaGLoRKsGJ1EOsuel8sl5
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-12_11,2022-09-12_02,2022-06-22_01
X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/12/22 4:14 PM, Lee Jones wrote:
> On Mon, 12 Sep 2022, Yonghong Song wrote:
>> On 9/12/22 2:38 PM, Lee Jones wrote:
>>> The documentation for find_vpid() clearly states:
>>>
>>
>> No need for the above extra line.
> 
> The intentional blank formatting line?
> 
> The commit message would look worse without it.
> 
> Is this really a blocker?

Not a blocker. Just a suggestion. I won't enforce it
if you do have this extra line.

> 
>>>     "Must be called with the tasklist_lock or rcu_read_lock() held."
>>>
>>> Presently we do neither.
>>
>> Presently we do neither for find_vpid() instance in bpf_task_fd_query().
>> Add proper rcu_read_lock/unlock() to fix the issue.
> 
> I can expand this.
> 
>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> Cc: John Fastabend <john.fastabend@gmail.com>
>>> Cc: Andrii Nakryiko <andrii@kernel.org>
>>> Cc: Martin KaFai Lau <martin.lau@linux.dev>
>>> Cc: Song Liu <song@kernel.org>
>>> Cc: Yonghong Song <yhs@fb.com>
>>> Cc: KP Singh <kpsingh@kernel.org>
>>> Cc: Stanislav Fomichev <sdf@google.com>
>>> Cc: Hao Luo <haoluo@google.com>
>>> Cc: bpf@vger.kernel.org
>>> Fixes: 41bdc4b40ed6f ("bpf: introduce bpf subcommand BPF_TASK_FD_QUERY")
>>> Signed-off-by: Lee Jones <lee@kernel.org>
>>
>> Ack with above a few suggestions for the commit message.
>>
>> Acked-by: Yonghong Song <yhs@fb.com>
>