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;
From:   Song Liu <songliubraving@fb.com>
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     Song Liu <song@kernel.org>, Networking <netdev@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        "jolsa@kernel.org" <jolsa@kernel.org>,
        "mhiramat@kernel.org" <mhiramat@kernel.org>
Subject: Re: [PATCH v2 bpf-next 3/5] ftrace: introduce
 FTRACE_OPS_FL_SHARE_IPMODIFY
Thread-Topic: [PATCH v2 bpf-next 3/5] ftrace: introduce
 FTRACE_OPS_FL_SHARE_IPMODIFY
Thread-Index: AQHYdrjyXTiiEgEgK0K1BTZfHuJgSq19RUuAgAGMyICAAAmfgIAAFU+AgAALzACAAADvgIAA+XEAgAAY+YCAAApDAA==
Date:   Fri, 15 Jul 2022 19:49:00 +0000
Message-ID: <0EB34157-8BCA-47FC-B78F-AA8FE45A1707@fb.com>
References: <20220602193706.2607681-1-song@kernel.org>
 <20220602193706.2607681-4-song@kernel.org>
 <20220713203343.4997eb71@rorschach.local.home>
 <AA1D9833-DF67-4AFD-815C-DD89AB57B3A2@fb.com>
 <20220714204817.2889e280@rorschach.local.home>
 <6A7EF1C7-471B-4652-99C1-87C72C223C59@fb.com>
 <20220714224646.62d49e36@rorschach.local.home>
 <170BE89A-101C-4B25-A664-5E47A902DB83@fb.com>
 <0CE9BF90-B8CE-40F6-A431-459936157B78@fb.com>
 <20220715151217.141dc98f@gandalf.local.home>
In-Reply-To: <20220715151217.141dc98f@gandalf.local.home>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?wcKEGZ8GG/QATwH/FX5jvhG5PFEOvTT5AGHJDda1862Qx4jMnZqw0bg6dhUV?=
 =?us-ascii?Q?RrhHrAI7CXRs6mpfG4oKv7SUqcxiL4fV8ZkSQFywJY09mfpaL3Z3e+D5o15j?=
 =?us-ascii?Q?NMIC5IgeoisWBmdxJsCml2ivu9nuomdx4MPEeLCA7W6PtC6uYVXruWRkXk3/?=
 =?us-ascii?Q?1fzixYL+/+6dP9TeXS9is9Hw4RkVcomMSOo69IFonoVNIgvw5XVphOvJis6I?=
 =?us-ascii?Q?Q+0sfEc+NozrUrOz4MXaJmimPrpMA2Mudo6C8e6ayFuXOgLhfYUdkTS2vTTo?=
 =?us-ascii?Q?49tDQSTgEvR9ah4adgCkZhvDOPRsv2gm1fGQH8kuWNMV1qLFAM/ZyI6gVoBK?=
 =?us-ascii?Q?OkhgkfhrKzQ+KAdbjgB122TMj/gj5TRD6OI5nSIDQ2db0mE02M8MXhWM372J?=
 =?us-ascii?Q?ZR31usKqlpkq07km2bKQVYlays949usr2WIFOfl2QMKcOKe1VCZO3ZlC+sj4?=
 =?us-ascii?Q?UBkgMG0BGm7PUIFO1I0bPrc7N1Y5SWxdX+an6Eovhayenrtx13cjCvfccO21?=
 =?us-ascii?Q?Evg4oX8r5PKIEgUBltI2YxJ8x4OtnLs7ITkb9hK4BY9qEIvA2FcZrEsetl6L?=
 =?us-ascii?Q?xkB0f+HYzQMtRhfdNP1dK4wE5dbbM+Dntgp8e9rPMNcFSpfI66IiaBfEP8Zu?=
 =?us-ascii?Q?4pGBkfSoL1W/b0ruP15kdcSzacbyokUEhGom3PrCVwkF061tBJFKB6zmz6RR?=
 =?us-ascii?Q?nvvcyyQ74vSpnwkG1Ye4gUQEjjFZUuFT2KZG6VQkUC/cWACn6fIwSABFqlkh?=
 =?us-ascii?Q?Ps6lNUjjCSdXbFwf57lOZ6ZTFw7KkDYRipxP6rsp/9yb328oRuaHlxkoU9GE?=
 =?us-ascii?Q?b9WyekuUE3zI/lfWdwGmhBuA55MJ29Bykmeez5drJRebkS/f4lTk0Vwhrbln?=
 =?us-ascii?Q?hdgHZVyyz1BeY1iwBJqV2Tevq3G4jUqYNQXmH/cK9WOlfNG5zWdVzZcoWuPN?=
 =?us-ascii?Q?tVJu6kKcFKWDE7gXAOddLuY9rZyFo1x81Rc9qDFb6mItqIPqBwVrfkFyFjw2?=
 =?us-ascii?Q?1X/E25UGuR8WBvGk33fYb6jYCF1Vn5fL7B7k0Iz75GOYyZmxLpUI/P6OaJwl?=
 =?us-ascii?Q?3Nd8Iq6rYqjqwkFVRkJLYOeWcrfjnt4Rtxiues2/+mZ1aU5t/2iHgLNsJdU0?=
 =?us-ascii?Q?slcygmc+1mzk4PFBDBa+VPMup0hyY5Loh7yZUX973HJOwEoU6ipzyRj2fT8F?=
 =?us-ascii?Q?+5V2+mD/kXqkhbGytotyDXagrZpuO+0UzoLb9b5jaxVz39gTi52L4ZWsBuqn?=
 =?us-ascii?Q?zuqcHmf7ZTRb2TH3ox1FtSuMcIkIqHKibUkNX0m9sZi+RWvasG6sgD3acPVt?=
 =?us-ascii?Q?C3kVOHzz1Wh/UlIPOpid9rxVWu0uRyUaTBbnlKRPDqlfw31jQL6+OhU+tUgf?=
 =?us-ascii?Q?WjdPXolc3lB2vyd3wB3Iq1uGSrtth1ef/HCmnAcZeaelTekJmN9VPJYOKH6p?=
 =?us-ascii?Q?pvJDGby1rndQmEEjhn+yWOZ9lPAA6t0FoJX4P2EP252l4VhvNN63uTPFgSSE?=
 =?us-ascii?Q?UcVJr3MQtDr9AovqW5A5lOsBULlbYQC4VG3NfOjC7mJfXfMjna7AZLa//aHD?=
 =?us-ascii?Q?4yEjteCA7amaqITYQV0YfDP77/zlmjBlzuUPD7+0IGVANZhepg/kUTM1OVj9?=
 =?us-ascii?Q?PykQUU4xZfb3aj7RnBmj1Is=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <359A9C26BFEA054799E522BF7D7B427B@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f71ee93c-632f-4f7e-f8b9-08da669b107d
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Jul 2022 19:49:00.8845
 (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: Vnvz37SH1LzBdBK57+aYtwknYSdFj+JIVJYCenYkW65pCkW74M9jt+BZwmzMqPRZHYVxY0u8w09URXh9e1SDKg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY3PR15MB4835
X-Proofpoint-ORIG-GUID: 9guEJhtX4X9hlH3oFfh8wCanAI8dGN4b
X-Proofpoint-GUID: 9guEJhtX4X9hlH3oFfh8wCanAI8dGN4b
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-07-15_10,2022-07-15_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,RCVD_IN_DNSWL_NONE,
        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 Jul 15, 2022, at 12:12 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Fri, 15 Jul 2022 17:42:55 +0000
> Song Liu <songliubraving@fb.com> wrote:
> 
> 
>> A quick update and ask for feedback/clarification.
>> 
>> Based on my understanding, you recommended calling ops_func() from 
>> __ftrace_hash_update_ipmodify() and in ops_func() the direct trampoline
>> may make changes to the trampoline. Did I get this right?
>> 
>> 
>> I am going towards this direction, but hit some issue. Specifically, in 
>> __ftrace_hash_update_ipmodify(), ftrace_lock is already locked, so the 
>> direct trampoline cannot easily make changes with 
>> modify_ftrace_direct_multi(), which locks both direct_mutex and 
>> ftrace_mutex. 
>> 
>> One solution would be have no-lock version of all the functions called
>> by modify_ftrace_direct_multi(), but that's a lot of functions and the
>> code will be pretty ugly. The alternative would be the logic in v2: 
>> __ftrace_hash_update_ipmodify() returns -EAGAIN, and we make changes to 
>> the direct trampoline in other places: 
>> 
>> 1) if DIRECT ops attached first, the trampoline is updated in 
>>   prepare_direct_functions_for_ipmodify(), see 3/5 of v2;
>> 
>> 2) if IPMODIFY ops attached first, the trampoline is updated in
>>   bpf_trampoline_update(), see "goto again" path in 5/5 of v2. 
>> 
>> Overall, I think this way is still cleaner. What do you think about this?
> 
> What about if we release the lock when doing the callback?

We can probably unlock ftrace_lock here. But we may break locking order 
with direct mutex (see below).

> 
> Then we just need to make sure things are the same after reacquiring the
> lock, and if they are different, we release the lock again and do the
> callback with the new update. Wash, rinse, repeat, until the state is the
> same before and after the callback with locks acquired?

Personally, I would like to avoid wash-rinse-repeat here.

> 
> This is a common way to handle callbacks that need to do something that
> takes the lock held before doing a callback.
> 
> The reason I say this, is because the more we can keep the accounting
> inside of ftrace the better.
> 
> Wouldn't this need to be done anyway if BPF was first and live kernel
> patching needed the update? An -EAGAIN would not suffice.

prepare_direct_functions_for_ipmodify handles BPF-first-livepatch-later
case. The benefit of prepare_direct_functions_for_ipmodify() is that it 
holds direct_mutex before ftrace_lock, and keeps holding it if necessary. 
This is enough to make sure we don't need the wash-rinse-repeat. 

OTOH, if we wait until __ftrace_hash_update_ipmodify(), we already hold
ftrace_lock, but not direct_mutex. To make changes to bpf trampoline, we
have to unlock ftrace_lock and lock direct_mutex to avoid deadlock. 
However, this means we will need the wash-rinse-repeat. 


For livepatch-first-BPF-later case, we can probably handle this in 
__ftrace_hash_update_ipmodify(), since we hold both direct_mutex and 
ftrace_lock. We can unlock ftrace_lock and update the BPF trampoline. 
It is safe against changes to direct ops, because we are still holding 
direct_mutex. But, is this safe against another IPMODIFY ops? I am not 
sure yet... Also, this is pretty weird because, we are updating a 
direct trampoline before we finish registering it for the first time. 
IOW, we are calling modify_ftrace_direct_multi_nolock for the same 
trampoline before register_ftrace_direct_multi() returns.

The approach in v2 propagates the -EAGAIN to BPF side, so these are two
independent calls of register_ftrace_direct_multi(). This does require
some protocol between ftrace core and its user, but I still think this 
is a cleaner approach. 

Does this make sense?

Thanks,
Song