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: <40fc10d4-b68d-83c3-b659-e291031df5bd@meta.com>
Date:   Tue, 30 May 2023 22:38:29 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.11.2
Subject: Re: [PATCH v2] kernel: bpf: syscall: fix a possible sleep-in-atomic
 bug in __bpf_prog_put()
Content-Language: en-US
To:     Teng Qi <starmiku1207184332@gmail.com>
Cc:     ast@kernel.org, daniel@iogearbox.net, john.fastabend@gmail.com,
        andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
        yhs@fb.com, kpsingh@kernel.org, sdf@google.com, haoluo@google.com,
        jolsa@kernel.org, davem@davemloft.net, kuba@kernel.org,
        hawk@kernel.org, bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
References: <20230530070610.600063-1-starmiku1207184332@gmail.com>
 <4f37f760-048b-9d54-14ae-d1f979898625@meta.com>
 <CALyQVaxuONP8WXSVGhT2ih12ae0FwE3C+A1s4O7LArTHERmAxg@mail.gmail.com>
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <CALyQVaxuONP8WXSVGhT2ih12ae0FwE3C+A1s4O7LArTHERmAxg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MVpMRDhxalJPS0NXazMzQzJ4My9ueERQeDI3cDZubmFHOW5SMVBoeDJ1NDlV?=
 =?utf-8?B?blZyV0w4OVYwbHE0WW5TQ3I3cVlpeUJlZG1NVWVWZUdCSTV3d2w2OVBQNng4?=
 =?utf-8?B?N2JjSjh5QW42bkg3aE5SV0xVSUhpa0xTK0RMMkhESU9ldnhRMEdrTm1IWmRh?=
 =?utf-8?B?Qzd4ZDBURWpnTHRDSWJvRDVkVEhCUWlnVmFEMFoyUUwzL3pEUHRkUTZWNHdB?=
 =?utf-8?B?T3BMSGpDQ21pZVdNVE40T25FVzRBSmF0eW1WM1IwcFV5NmZnZGlpMHpDREhU?=
 =?utf-8?B?QkFUMkVPeVhXeXFBQnl6MERhT1dzalpEclBrMm9EdnczaUxjQWhKWFJzcHBQ?=
 =?utf-8?B?UlRFcis1SXFuS0REZFYxSzFUalhVdllqZ1ZzbDdLZmtEZmFrSXE3MnlrRnpq?=
 =?utf-8?B?bDNHU1E0NGFNbW9BWkJkZXBiaFVFNXd3eTFibkpPbHZSYURuVW5FdURCODls?=
 =?utf-8?B?K0grVVQ4am5jSGgzN21YSGVDNm9aQW44WWNJVFZUZzMzRVFnRG9OeTNDVmx3?=
 =?utf-8?B?bzQ3VmRuSVN0TE9Ic29paFpmaGptZzk4YWF1cnh4aVBaOFFlVHFnYzY0dzRr?=
 =?utf-8?B?VVNJa291U1ZYRFIycXBoSDJXVmhnWjJpeEp6d1F0aTRoR09VWVNnOGZaMlJ3?=
 =?utf-8?B?M1d1Vmc3RUF6WTBzTW5HMTkzWkt0RnN2QXM2OUYxTXI2cGtua3NGT2dZck9X?=
 =?utf-8?B?R2E5UXY0RjlTdkJIc1JSNjhtZ0FQRnl6cjkyYzhNNHNCc21uQ0tFdE03Nzgx?=
 =?utf-8?B?a2N5Z1Rod2tnZ0lPcnY2Q1NGRkMvejhrYUpFUkpHeEo3WFcycXEzc1RRSXkx?=
 =?utf-8?B?TUdkcUJuM2E0QldacDR4YW9JWFgxZUtVbWdicWV4QUhNOWlFZEYvSWR3MXQ2?=
 =?utf-8?B?dUFGbEk5dUR2QXJxSGV2N3R1YlRmWFpVNHhFTjBnMk85djJkV2ZuY1FkbFVi?=
 =?utf-8?B?WmJ5Smk1ekR6NGxsRS9uekxaTCtkdk9wVHkySTRxcGdmSnFneGxhQWZzUUpO?=
 =?utf-8?B?OU1xUUZNU242dHAxZzFZUjhSc3FzQi8xdGFsZ1NvVXYxZGhhdG00bzI4alpU?=
 =?utf-8?B?d01vNDJvVGRzMEdxeExRRldRRDZORkM0SXk3elpKSXlmM21VSld6UzRLTkNh?=
 =?utf-8?B?WVZrSWREdnVUZUxpajFYdW5IbGVJSGlwWlRXNS9oNEdJWFMzaHJEd2MxZFVK?=
 =?utf-8?B?ZXovUGlTOEg5aHRBNUNkQS9ieUxINzFEaUd6ZFVjRUhZZy91dktPdnJhRjI0?=
 =?utf-8?B?S2lLaWhFSUFJUkorcHpUUlUxbGpVK3dHaTdYc2NJV2lOYzlLa3lUdjdNRCt1?=
 =?utf-8?B?aUMxS2VzTkhSdHhYMXZoOHlLNkV2ZTBVUHdqTFVxdEdKRkFwY3BGY0xIZWhF?=
 =?utf-8?B?a3VQM3RSOXVOQXRIM2dRSThod0RiWm9EVFk4OFlrM2VKSTVpb3VRcFgwRG9Z?=
 =?utf-8?B?NkNrVDdHTVVyU0VGcWwxdXpITUpqSEVjazhCUHZodkU1WjhyUm1LalFka054?=
 =?utf-8?B?NW9TelQ0aEUyTTZyaExaRC9BZXlZRVpkOGwrU3I0cEFRR3d2L1laN09sbGJl?=
 =?utf-8?B?L2tJVlpiR3ExcUh3ZmZCVHUveFdpNjFjTktKWU5McXhSaHNPWnBVaE5EQXFk?=
 =?utf-8?B?WTBGc1VoaFlqSERtYThXTmdnYmVLS2szT2puZ29XbTBMbmFNSXJvenFJbWhV?=
 =?utf-8?B?aFNvcUhpQXZidTFJLzVDODhnV3REVUtkcGlDRUhRbjhtMWJudkF3MkFwcndW?=
 =?utf-8?B?MXVuNG50R1l4ZDBLcS9aVS91VE0raTBxbHJHYU03R2N6ampIQmtjZy9ieXlQ?=
 =?utf-8?B?TW91bFZxT3lIRjZlWkZySi9SOWl4RFczZ3Y4a2JrZ2w2aC90ZHdkUWpBVmZU?=
 =?utf-8?B?eGpSeEFFLzBJVk9YUmxnZmZKbVhjSGNPRDAyQXUwNmZidVEvMmR2R21iS2I4?=
 =?utf-8?B?Z2cvNEVWV3hsUytxQS9CT3FlODlIMlZ6RW9OeW5UcFlpdjYyNEgxMTd1Qk1B?=
 =?utf-8?B?ZWkxMFU4dmdqSDRZZFgvMEN1TWh3TnZBNmQ5c2JGTlY1Mk82UnhwT0xVVjdD?=
 =?utf-8?B?NkEzODI1TXIrL09pU0FvWGlMRmFtVlJackpmZHpGeDdWYnlSbk9iTFNDdXNv?=
 =?utf-8?B?a1hOQVVYWGRnTCs3UmFxYjl1SnFOSlFLRC83RE5yc0RhSHFYMDdIQi9qY1hs?=
 =?utf-8?B?ZGc9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9e8c91e9-4ba1-45c7-6c9a-08db61994636
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2023 05:38:34.0795
 (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: epeCF34dzA/aNAjkmwSmmlDjnW/KfrXvSXnIhSqzM8KhMDQLkS7onF6PYvacXvT6
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR15MB5764
X-Proofpoint-ORIG-GUID: qPhfV_vg7Dk9cIBM5WegSn6WFJrAgW1y
X-Proofpoint-GUID: qPhfV_vg7Dk9cIBM5WegSn6WFJrAgW1y
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26
 definitions=2023-05-31_02,2023-05-30_01,2023-05-22_02
X-Spam-Status: No, score=-2.9 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_PASS,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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 5/30/23 10:30 PM, Teng Qi wrote:
>> I would really like you to create a test case
>> to demonstrate with a rcu or spin-lock warnings based on existing code
>> base. With a test case, it would hard to see whether we need this
>> patch or not.
> 
> Ok, I will try to construct a test case.
> 
>> Please put 'Fixes' right before 'Signed-off-by' in the above.
> 
> Ok.
> 
>> Could we have cases where in software context we have irqs_disabled()?
> 
> What do you mean about software context?

sorry. i mean softirq context.

> 
> On Wed, May 31, 2023 at 1:46 AM Yonghong Song <yhs@meta.com> wrote:
>>
>>
>>
>> On 5/30/23 12:06 AM, starmiku1207184332@gmail.com wrote:
>>> From: Teng Qi <starmiku1207184332@gmail.com>
>>>
>>> __bpf_prog_put() indirectly calls kvfree() through bpf_prog_put_deferred()
>>> which is unsafe under atomic context. The current
>>> condition ‘in_irq() || irqs_disabled()’ in __bpf_prog_put() to ensure safety
>>> does not cover cases involving the spin lock region and rcu read lock region.
>>> Since __bpf_prog_put() is called by various callers in kernel/, net/ and
>>> drivers/, and potentially more in future, it is necessary to handle those
>>> cases as well.
>>>
>>> Although we haven`t found a proper way to identify the rcu read lock region,
>>> we have noticed that vfree() calls vfree_atomic() with the
>>> condition 'in_interrupt()' to ensure safety.
>>
>> I would really like you to create a test case
>> to demonstrate with a rcu or spin-lock warnings based on existing code
>> base. With a test case, it would hard to see whether we need this
>> patch or not.
>>
>>>
>>> To make __bpf_prog_put() safe in practice, we propose calling
>>> bpf_prog_put_deferred() with the condition 'in_interrupt()' and
>>> using the work queue for any other context.
>>>
>>> We also added a comment to indicate that the safety of  __bpf_prog_put()
>>> relies implicitly on the implementation of vfree().
>>>
>>> Signed-off-by: Teng Qi <starmiku1207184332@gmail.com>
>>> ---
>>> v2:
>>> remove comments because of self explanatory of code.
>>>
>>> Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq context.")
>>
>> Please put 'Fixes' right before 'Signed-off-by' in the above.
>>
>>> ---
>>>    kernel/bpf/syscall.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index 14f39c1e573e..96658e5874be 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -2099,7 +2099,7 @@ static void __bpf_prog_put(struct bpf_prog *prog)
>>>        struct bpf_prog_aux *aux = prog->aux;
>>>
>>>        if (atomic64_dec_and_test(&aux->refcnt)) {
>>> -             if (in_irq() || irqs_disabled()) {
>>> +             if (!in_interrupt()) {
>>
>> Could we have cases where in software context we have irqs_disabled()?
>>
>>>                        INIT_WORK(&aux->work, bpf_prog_put_deferred);
>>>                        schedule_work(&aux->work);
>>>                } else {