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: <53f21d98-4ee6-c0e9-1c0a-5fae23c1b9a8@meta.com>
Date:   Mon, 12 Dec 2022 09:52:03 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.5.1
Subject: Re: [PATCH hid v12 05/15] HID: bpf jmp table: simplify the logic of
 cleaning up programs
Content-Language: en-US
To:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Jiri Kosina <jikos@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <shuah@kernel.org>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     Tero Kristo <tero.kristo@linux.intel.com>,
        linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        bpf@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-doc@vger.kernel.org
References: <20221103155756.687789-1-benjamin.tissoires@redhat.com>
 <20221103155756.687789-6-benjamin.tissoires@redhat.com>
 <CAO-hwJ+fYvpD5zbDNq-f-gUEVpxsrdJ7K-ceNd37nLxzBxYL+g@mail.gmail.com>
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <CAO-hwJ+fYvpD5zbDNq-f-gUEVpxsrdJ7K-ceNd37nLxzBxYL+g@mail.gmail.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?WkhtSm9OWXVZcGdmb29TMUgxM0JOUGlMWmZCcC9vSVA3ZndaSnV1UThJWGZJ?=
 =?utf-8?B?TGtBbXZ4UmQ1UHhoQ0tscjVkQkJLdHlBbHU4ZXVCR2swRlg3V3FyVzVOZGM3?=
 =?utf-8?B?QjVWcTVrWDNqTnZjNEJ5Z1hxbHZtdDBSUGZoWjUrRS81RHdETmpXTmFOcXZN?=
 =?utf-8?B?c0NxanNMOC9yZWxCamIxWTFQM0Joa29ja1o4V2h2aFB5VENweGpvOUFlRWNz?=
 =?utf-8?B?dHQ5SVF1RGc0VEp0ekJJemkvOGw2YmlzcFJ6M1BtdFEvYzdPdnZoMEh4UUtO?=
 =?utf-8?B?UVdKMTZ1UVhMMFJ3V1FSUTBHTGVTeTFjL0o1djFhaEFPUGVvVjRHQUFIOFkr?=
 =?utf-8?B?bGJFdVFLL3pTY2I2dzl5YitoQ0VxQVQrMFUwQldrNEVGK3J2MkJ6aWJYMk82?=
 =?utf-8?B?cEZIOE9UZ3ZONUVMaThaVVJPMG04WDFiczF3VHdVOEhVaG5LZWVwS1VVR1Fr?=
 =?utf-8?B?SjBOdnFOc2xnZUNaejZqaGt4UlMwNGtoN1kwdjlrUXo1MmdtemZsWWs2a1hS?=
 =?utf-8?B?Yk1SMVY0YzlRVlBKZG45bVE3a2g3bEN2aStGdkl2cFZpSGV4NDFsbzhYVGVx?=
 =?utf-8?B?cGhOKy9EWjk4VTY2S1NhY0JYY01RN1RLdEdrbTUyWlF2VTcrR0xiaWxLaFd2?=
 =?utf-8?B?TTZ6d0oybHVPUTYzbCtrN041bWg0a3d1RUxxazJ3OCsvMjUwWXBVL25GZ00w?=
 =?utf-8?B?ZG9XL1o4MEhlbTFwcDhZQnBNZ2ZuQ0dtQTJZOGxWOWJyOTJmblZkdVdmaVUy?=
 =?utf-8?B?RERWQU1qNjlhMTlDVkIzcVo1Z01pUjRtWHJVMTF3MnNndDhZbUJvRjQvdTdW?=
 =?utf-8?B?bVVaRFRlWE85YUF0anh2eTNTRnQvcXlITTRubjlqTW5WWmZMWW83UUwxQ2NV?=
 =?utf-8?B?WEVXNzZoOFhBN0pGQnBtT3NXdDZRVGsxZ3lYU29ZeXdCMEk2czFmZDZ4ME1T?=
 =?utf-8?B?bHUyZzZiMXozc2k0bHdOZ2ViNUpXRE1zREgxVUtwTWVIWGcvY0ZrRkdyNitj?=
 =?utf-8?B?R3U0WFFSdUdKYzA5bUlGYVk1RnJVSXdwdFVZcjQxV2hncHRIU2JyRzF6S2U5?=
 =?utf-8?B?eVV3U2FyNDZBTllvQWRQN1hEZVJPMC9KZUJKclMyNDU3RWkxK1NRUlRvQzNJ?=
 =?utf-8?B?U3UxS1o0QTZrMWNYM0hDR2xuNlB1N3VEb3JXOGJRYS9VUmlCZXBuNTdIKzdV?=
 =?utf-8?B?NVl2ejEySjZFL1h1bU5MMWFMcFg0aGxDU0JYUUhIdzQyaE13SmN2SVZEa2hp?=
 =?utf-8?B?UU1LVEhscjM2S0RBZThRZ1orL25Hc25odC9uVGt2MVNnaXgxOGgzQ0dQblNT?=
 =?utf-8?B?UWx4ZWtKdlhqYXdTbFA1LzVJR1QzWE1MVHJycG1ka2pmZDV1LzBmWHAzSkM3?=
 =?utf-8?B?SkZQSi9wV2RpUnE2Y3RXOFN2eEpEMnFkWHZ3Ui8rUVgydHI4T2kwRzVqcnFO?=
 =?utf-8?B?UEhHQjdDc2NLZ1JsY3dxNXdWbEpLb1RtZzBOUXdNR0JhL1czVHQyM01vZzc3?=
 =?utf-8?B?MXhsR3hyNXZVRHlBOUZYOEgvUUtFd0h2S08xK0RLVlh5ZVRVN216N09PN0d1?=
 =?utf-8?B?dk5oY2k0SE02V0RoeitMcHVVYzVTWnY3bVRzbS9sRXRkWTRkblMweXNWZm9M?=
 =?utf-8?B?UFBlcFd2dnlsSVc1Q1hlWlYzV3JENnl5YXhjdXFreXBZOTlYR3NuZ2tFSEcy?=
 =?utf-8?B?cUtUQklVa2Jwa0phVUN3eWh2c0RBRjZEeHd5L2E1U2p1NUFYMkc1cjVMY1Fs?=
 =?utf-8?B?TkM5dTZEdU1qRTNlQ05pZTJCRWg2c1ZjOHBxMXJTNE9kNG12SzFuWnVOSENR?=
 =?utf-8?B?SkpOdnU4OWpma1FqU3RrRTU4UUZzUkt3VEJsb1orN2Q0SGQvalJtTS9lQXc0?=
 =?utf-8?B?TTdiNXlmcGd6amM3YVp2ZHI3VE9HYzMxb29PSm1xMHMwN0RtdDUzb1M1UDh5?=
 =?utf-8?B?M0NSQXorWFpmenQycEpkbFNDbzVCTEdSTkZoMitjZzVZanNKTkY0VkZEdW4z?=
 =?utf-8?B?RnRPME05c2p5RFl4T1NMUFlWZXB5aTB5OEUzQ041OFpkcHBzZVJzU1B0SEkw?=
 =?utf-8?B?OTRxYmxESWkrbkhxKzlvUTJxUzNXN3dNN3k5dEVlRXVnV21Bd0h3ZEZXcWQv?=
 =?utf-8?B?UktxOXlLcXFsK3F4c3ZkWENLYjZhK05pM3c1QkxwRGgwUjNLQkxKR0hjUnFu?=
 =?utf-8?B?UXc9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 694a8e57-fb17-4621-b830-08dadc6996bf
X-MS-Exchange-CrossTenant-AuthSource: DM5PR1501MB2055.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2022 17:52:08.6263
 (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: HPYtoO1MGBBTIxeLkVTT6IWspO5BKPvAUeqzJev71IDhRk5D6ZcVM7AZpXES24sh
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR15MB5396
X-Proofpoint-GUID: 9C9ik_E1PgQW2jEJgk11QgFG0e1Kgi3K
X-Proofpoint-ORIG-GUID: 9C9ik_E1PgQW2jEJgk11QgFG0e1Kgi3K
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-12-12_02,2022-12-12_02,2022-06-22_01
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        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 12/12/22 9:02 AM, Benjamin Tissoires wrote:
> On Thu, Nov 3, 2022 at 4:58 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>>
>> Kind of a hack, but works for now:
>>
>> Instead of listening for any close of eBPF program, we now
>> decrement the refcount when we insert it in our internal
>> map of fd progs.
>>
>> This is safe to do because:
>> - we listen to any call of destructor of programs
>> - when a program is being destroyed, we disable it by removing
>>    it from any RCU list used by any HID device (so it will never
>>    be called)
>> - we then trigger a job to cleanup the prog fd map, but we overwrite
>>    the removal of the elements to not do anything on the programs, just
>>    remove the allocated space
>>
>> This is better than previously because we can remove the map of known
>> programs and their usage count. We now rely on the refcount of
>> bpf, which has greater chances of being accurate.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>
>> ---
> 
> So... I am a little bit embarrassed, but it turns out that this hack
> is not safe enough.
> 
> If I compile the kernel with LLVM=1, the function
> bpf_prog_put_deferred() is optimized in a weird way: if we are not in
> irq, the function is inlined into __bpf_prog_put(), but if we are, the
> function is still kept around as it is called in a scheduled work
> item.
> 
> This is something I completely overlooked: I assume that if the
> function would be inlined, the HID entrypoint BPF preloaded object
> would not be able to bind, thus deactivating HID-BPF safely. But if a
> function can be both inlined and not inlined, then I have no
> guarantees that my cleanup call will be called. Meaning that a HID
> device might believe there is still a bpf function to call. And things
> will get messy, with kernel crashes and others.

You should not rely fentry to a static function. This is unstable
as compiler could inline it if that static function is called
directly. You could attach to a global function if it is not
compiled with lto.

> 
> An easy "fix" would be to tag bpf_prog_put_deferred() with "noinline",
> but it just feels wrong to have that for this specific reason.

This is not a right approach just for this purpose.

> 
> AFAICT, gcc is not doing that optimisation, but nothing prevents it
> from doing it, and suddenly that will be a big whole in the kernel.
> 
> As much as I wish I had another option, I think for the sake of
> everyone (and for my future holidays) I'll postpone HID-BPF to 6.3.
> 
> I actually thought of another way of removing that trampoline call. So
> I'm not entirely going back to the drawing board hopefully.
> 
> [a few hours laters]
> 
> Just as a preview, I am reusing the bpf_link idea: when we call
> hid_bpf_attach_prog(), this creates a bpf_link, and that link is the
> one that needs to be pinned. Whenever all the references of that link
> are dropped, I get called in the link's ->release() function, and I
> can force the unbinding of the hid-device to the program at that time.
> 
> Way safer (no refcount mess up) and no optimisations can interfere,
> now that I am not "tracing" the bpf core code.
> 
> Cheers,
> Benjamin
>