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: <5aa15b12-d193-b505-5786-3e187751323e@fb.com>
Date:   Wed, 10 Aug 2022 08:09:09 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.12.0
Subject: Re: [PATCH v2 1/1] bpf: Drop unprotected find_vpid() in favour of
 find_get_pid()
Content-Language: en-US
To:     Lee Jones <lee@kernel.org>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     LKML <linux-kernel@vger.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>, Jiri Olsa <jolsa@kernel.org>,
        bpf <bpf@vger.kernel.org>
References: <20220803134821.425334-1-lee@kernel.org>
 <CAADnVQ+X_B4LC6CtYM1PXPA4BBprWLj5Qip--Eeu32Zti==Ydw@mail.gmail.com>
 <YvIDmku4us2SSBKu@google.com>
 <CAADnVQ+5eq3qQTgHH6nDdVM-n1i4TWkZ35Ou8TDMi3MqGzm63w@mail.gmail.com>
 <YvOQhTUD1x6W0ozO@google.com>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <YvOQhTUD1x6W0ozO@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?U1NDN3VJU1J5TXdndFREaWNjMXRpOU1CZXQyOGZkeE1JSmhtNWxlUXZJWEVS?=
 =?utf-8?B?b1BsRVExZldGcUJzMjNTY2JQQVJaY0Y2NWpaQWxTNThQYTl0RkdHQlFBelFq?=
 =?utf-8?B?Y0xBb0NBcE9BaStvalM1SCt2MHZSV1gzdk9Dc1B5UVdBZm5ncXhrOWFUQzM4?=
 =?utf-8?B?Zlg2MHJtbWlVQzBjMUlDOWVFQ1cxVzN0VWIxb3pFTWh3TmJ5WFlvYkI3NlR4?=
 =?utf-8?B?bUQrN2FoeTd5ZVc0S1lBVE81ZjN6TjBqWm9adXpsejArQ2Zid2JSTVpyNXhS?=
 =?utf-8?B?VmdyK2FPcGtKaGZHNzlGWjViMnF1MDRtQi9IN3pmU1d0Smx3bncwRHcxMW9R?=
 =?utf-8?B?ck5uU3lQeWZXa3lIY1hvNjh1TnBGRXRaYnk3ZGxhMFFDczkyL2FMc3dhYjNT?=
 =?utf-8?B?bFJKYlRyNjgvWmhTWG5SOW1jTzVQZnJFSVR2L3ladU1zQ1JkalNscHVsaEtn?=
 =?utf-8?B?Nzk2MjhyS2VBOXFTSnhjNWF4VzZoK2lkOEFMUVBSQ1RpWXdFTkRlSHRwcXdy?=
 =?utf-8?B?M3FQK2pZemMwK3UwM1VQSkIxS1BPNGRCMUhUdEREWVVzUjE3S1pLMXRlR3BU?=
 =?utf-8?B?NjYyanA0bmlHNlF4V3dLcjBHR0VVOTJKMzBYVFpDRjM0d2o2SWJIckZHU3Qr?=
 =?utf-8?B?NGFIc0JkUHV6bm42bDBZTW9DZFozdkZHNDRBKzhLeENXSUZVOXFwWElRTzRV?=
 =?utf-8?B?U0V4ZVFFSVE0K1dYWEF1VDdQekhvMExGbThvN0M3UER4aXFMS0FYL1BIV2k1?=
 =?utf-8?B?UE5LbXpqRDVLRGNTTHdjU0lzcWs4a3hXNVB2QVNDdmxic1I4NFVGOFdwdFRQ?=
 =?utf-8?B?THJ3V1pLaHBlKzRCYjJ2L0V0eEVFQlBZR1NuSmk4dlpWVDZUbTFyZDQ1amRT?=
 =?utf-8?B?UFI0Nm1nSkJOSmVxL0x3cUhwOHNESzFTUGd4ZWtNYnFyc3MrU3Z1NXY1cE5D?=
 =?utf-8?B?SERveGxZQlo5UkFvblFLOEdyT3I3NGtaMktjQkZGTHRjWXlWZzJUeFRIejZH?=
 =?utf-8?B?amxMdzNpMUltWjZwUUJTZHpDMHdlVGlqL1NtbjRRZE00VkRjQ1Rsb05BQ0I4?=
 =?utf-8?B?Mnlud3R0Vkk4N3I1dTVKTk1WWE05dVZROTNjOXhKcTRiV0E3bGVJSVZTNjYz?=
 =?utf-8?B?WGp4WUVXMzJLV2V4c2JyZ0lZaHRFejNvYzZFcGREMXliemk2dXRQSHlLbTcr?=
 =?utf-8?B?ektHWFRGUG0wT1BBSjVRcm5TeHloR3ZXKzYxdW1wRkdBdGJ1TkQ3UGtGY2l2?=
 =?utf-8?B?bXZibThzZ0l0OUdOYjlxMXRDZ0hZbEt1SittZmJEODBmakZYMlN4UElxVml4?=
 =?utf-8?B?MW9BeE1yOEg4SFdZWGQ3dG13ZDBoWEhDcTNidjVDRnVlS0trWncvSnI3TFBC?=
 =?utf-8?B?YnRnRHdEWTdHREF1alNRV2NscHFXLzdmdG1SYi95dzdJN3UvdGpJa0o1UTNN?=
 =?utf-8?B?N0QvYUgyU0tuY2hHRVpOQXZmc21SQ1lGbS9JWHo2WHpWRXVhWmZMOC9oZ1FD?=
 =?utf-8?B?OXBvb09VTE1JQThoQ1J6UmUrZldRSys5TnJZQTFSKzNNbEJiRDF5TFlKL1Zz?=
 =?utf-8?B?cUpnRXdldUNnQjBuNGlFTVdrZVhpNE1EVHZRL28yYnRnOHFmTG5TcUYxUm96?=
 =?utf-8?B?M2pqbzgrWlMvQzVJRDRkN1ZnZXFScVB2WlQ2eEpJNlU5RGdvblRsNEhrRDIw?=
 =?utf-8?B?OTlVVTVjWVZoQ0piYWxURGRTK3o1L3VVUkRjbVZ0YVJ4TXVIYmNCYXFHenh1?=
 =?utf-8?B?N0dFUW40RjRjSEF5dXBpZEFmWnk3ZkVZaGZSTi9kclNHYzlFRHVxUnhGRnZt?=
 =?utf-8?B?Z2E5dzNZajVlckkrKzlJVWtCNDdrZjNQdnF2czNoS0dsa0M3N1FuU1BYb3Bl?=
 =?utf-8?B?d1VNN1k1NkQxcW1XdEZuRG9iZ0l3WjVyaWt3WGh2Vk1OWFJWbGdUSXQ0TWVW?=
 =?utf-8?B?a0RITUZCVnNhLzhRUVR4Z0FLeW9FSUF4a2xldkdqQ2c0RWZoMU1iL0lkbDli?=
 =?utf-8?B?c3grM2FVRHZtR2NHNGo3dG1sVHlVYkd5KzF2VEZSUEd6T2RFYkhHeDlNWkRz?=
 =?utf-8?B?a0p6YVg4czdIVzVxV0VXamZLYi85MTIrVVFNR2xLSVIwZkdSNXdUc0cxaXdP?=
 =?utf-8?Q?ntQDtkakW+bbmkw/DB3WSkk59?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 46a88c04-a1b3-4763-7538-08da7ae24845
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Aug 2022 15:09:12.2711
 (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: 3fGCtwb9GHf+Y2/PQmn+yzYbfLHj9G82GNnMuVypAQkWalG1TMk7An30xxpfo1FN
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB3510
X-Proofpoint-GUID: qGDiUzLqS7oNlkqu7OOlRUQLQZxEtL_R
X-Proofpoint-ORIG-GUID: qGDiUzLqS7oNlkqu7OOlRUQLQZxEtL_R
Content-Transfer-Encoding: 7bit
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.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-10_08,2022-08-10_01,2022-06-22_01
X-Spam-Status: No, score=-2.7 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 8/10/22 4:03 AM, Lee Jones wrote:
> On Tue, 09 Aug 2022, Alexei Starovoitov wrote:
> 
>> On Mon, Aug 8, 2022 at 11:50 PM Lee Jones <lee@kernel.org> wrote:
>>>
>>> On Thu, 04 Aug 2022, Alexei Starovoitov wrote:
>>>
>>>> On Wed, Aug 3, 2022 at 6:48 AM Lee Jones <lee@kernel.org> wrote:
>>>>>
>>>>> The documentation for find_pid() clearly states:
>>>>>
>>>>>    "Must be called with the tasklist_lock or rcu_read_lock() held."
>>>>>
>>>>> Presently we do neither.
>>>>>
>>>>> Let's use find_get_pid() which searches for the vpid, then takes a
>>>>> reference to it preventing early free, all within the safety of
>>>>> rcu_read_lock().  Once we have our reference we can safely make use of
>>>>> it up until the point it is put.
>>>>>
>>>>> 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: Jiri Olsa <jolsa@kernel.org>
>>>>> Cc: bpf@vger.kernel.org
>>>>> Fixes: 41bdc4b40ed6f ("bpf: introduce bpf subcommand BPF_TASK_FD_QUERY")
>>>>> Signed-off-by: Lee Jones <lee@kernel.org>
>>>>> ---
>>>>>
>>>>> v1 => v2:
>>>>>    * Commit log update - no code differences
>>>>>
>>>>>   kernel/bpf/syscall.c | 5 ++++-
>>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>>>> index 83c7136c5788d..c20cff30581c4 100644
>>>>> --- a/kernel/bpf/syscall.c
>>>>> +++ b/kernel/bpf/syscall.c
>>>>> @@ -4385,6 +4385,7 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
>>>>>          const struct perf_event *event;
>>>>>          struct task_struct *task;
>>>>>          struct file *file;
>>>>> +       struct pid *ppid;
>>>>>          int err;
>>>>>
>>>>>          if (CHECK_ATTR(BPF_TASK_FD_QUERY))
>>>>> @@ -4396,7 +4397,9 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
>>>>>          if (attr->task_fd_query.flags != 0)
>>>>>                  return -EINVAL;
>>>>>
>>>>> -       task = get_pid_task(find_vpid(pid), PIDTYPE_PID);
>>>>> +       ppid = find_get_pid(pid);
>>>>> +       task = get_pid_task(ppid, PIDTYPE_PID);
>>>>> +       put_pid(ppid);
>>>>
>>>> rcu_read_lock/unlock around this line
>>>> would be a cheaper and faster alternative than pid's
>>>> refcount inc/dec.
>>>
>>> This was already discussed here:
>>>
>>> https://lore.kernel.org/all/YtsFT1yFtb7UW2Xu@krava/
>>
>> Since several people thought about rcu_read_lock instead of your
>> approach it means that it's preferred.
>> Sooner or later somebody will send a patch to optimize
>> refcnt into rcu_read_lock.
>> So let's avoid the churn and do it now.
> 
> I'm not wed to either approach.  Please discuss it with Yonghong and
> Jiri and I'll do whatever is agreed upon.

Hi, Lee, Let us just do rcu_read_lock() approach then. I am okay with 
that. Thanks!

>