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: <2c5669f1-b9d9-ee78-c5ee-d29a41d4d70a@fb.com>
Date:   Thu, 3 Mar 2022 11:13:47 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.6.1
Subject: Re: [PATCH bpf-next v1 1/9] bpf: Add mkdir, rmdir, unlink syscalls
 for prog_bpf_syscall
Content-Language: en-US
To:     Hao Luo <haoluo@google.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Andrii Nakryiko <andrii@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>,
        KP Singh <kpsingh@kernel.org>,
        Shakeel Butt <shakeelb@google.com>,
        Joe Burton <jevburton.kernel@gmail.com>,
        Tejun Heo <tj@kernel.org>, joshdon@google.com, sdf@google.com,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220225234339.2386398-1-haoluo@google.com>
 <20220225234339.2386398-2-haoluo@google.com>
 <7e862b1c-7818-6759-caf1-962598d2c8b3@fb.com>
 <CA+khW7gAEL+yBmXjWO28ns5hU4oHVZrEArfepuOfy6Q1y7VDKQ@mail.gmail.com>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <CA+khW7gAEL+yBmXjWO28ns5hU4oHVZrEArfepuOfy6Q1y7VDKQ@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?VFU1OFQ2VCtZV3A1czZ6Vzc2Y3JQV2tkaXUxZkN6ekZ3L2szMzVaNHUraHBs?=
 =?utf-8?B?L1FBWStKd1NqWnBWaVZjdDVNa05hdzBCcVNRWFMza2VXOGhLQTJUbk5qUE1R?=
 =?utf-8?B?a2YrYng2Mmh0TTZBVUhTeFFMeVhSRnhvS2RtVEo2STZwRnpSZ0dEa2NpdXV5?=
 =?utf-8?B?ZGd6TjBRUnBlckpCVDlYc1JvOVJIdEtmN2xvUWRUQm5JbG5xL1RSL2paaExB?=
 =?utf-8?B?NVRiZWpKNmY4Yy9PWnQ4ek5YamUzZ2x0UTY0QTJpak1vYkJhaUNGYlVzT29I?=
 =?utf-8?B?Qi82bWx0ZU5POTBpWFVKa1gxdlJSUHVTNU04TDJkdE9XTk5adHRCM2ZQWFJV?=
 =?utf-8?B?Y3VycDgwejJtS2R4eGtlWHNGSXdmRitkUzNNc05KQnhremxWOCtaaG0wSVBI?=
 =?utf-8?B?Q21NNEZsTmpZaE5PV1JFSXFQMEJTeDNqcmhXZnFRNWEyWTA4THh3WFNjR0ha?=
 =?utf-8?B?dXZ6NnZOZmtrNzQ5a0tWcGdNWWlicXRnWWZLNW9DRWpENnMrTGtGdGJMM2RF?=
 =?utf-8?B?YXpvYVJOQ1RsYkFVWkNjMEo3cUk3aWRSU3YxZUVzOTEwUXFOSk1rakswNkFY?=
 =?utf-8?B?STJaNFB0cXE0dnBoUHZ5akthMzFxMG5oQ3pOVVQvM1JERkZHVi9EMFViaUdh?=
 =?utf-8?B?ZGFkNXBHckJwQlhiR2VDUVNOWmt1bUxWTmMvd2JZc2dJejRIKzEzOGthZmpL?=
 =?utf-8?B?eTE4bndVWEo4ME5ENUVQa1R2V1Fab25hNS9BcS9nQXdHdzBZNS9ZZDZiL2xN?=
 =?utf-8?B?bUdsY3VWQ2JMTWFtbUpac2NZNm5jbER2cU5zUFN0V1k2cERPaktheUVDZUdN?=
 =?utf-8?B?cUJPeGtZejB0akJLYTBMUjFqNEhzVURVWGN0WUNrWnNRR1QwQVBOd1FIOGtL?=
 =?utf-8?B?VmFqVGdoU0VSd2cyQkFuYnlLdi84SXRyc0hxZnFzS25BVVJqbkVrd2d0NDUv?=
 =?utf-8?B?bmxxdjNtcWxYcVhlSW9lR0V2YkpUNyt5ZW9LT1FPSmw4SXg2MGZPb1V5Umpp?=
 =?utf-8?B?MXdwZnpiTXkwb1NKc0M5WC9ELzIzeWk1Tnc5amNNazRFdGxKeG5MVi81eVU3?=
 =?utf-8?B?WS9JVFBJcEVrYk9sZjFzdzNRZ2gxN0VicUpENUhHeUhoVGlmdkc2ZEFBYjhl?=
 =?utf-8?B?V1E2bit5N0F6V2xoQVBPZWppSjN6WVpXVXBiUnQ2WjNtd0RIMDBUQjd1azl3?=
 =?utf-8?B?Rm4wOE44SThLeXE2V0M0OThXVkpaam1oUmRObE81eVN1bVQxekkrYjUvN09v?=
 =?utf-8?B?Uk94VFQvaHVSUm5xeTdEQ3ZWR01WbmJrNi9QclcyUGthZnQxbWVNa0hKdm5H?=
 =?utf-8?B?TkxnYTBpSkNCemE5RGVoWGJKK0x4Qk1Rei8rUnJkWE53dE9YaGZvdGRLSkNV?=
 =?utf-8?B?NFhkb0d3ZllVU29jcDB3SjZ5T3Vka3RGTGhUdzJSbjY4RUZuZG5kYW90aHlD?=
 =?utf-8?B?TkJOVHZ1dW5qSGNHSjRwUlJrOGcxVlNIV2ZmYTd4clJDY09GZnFOWEdhU3Er?=
 =?utf-8?B?QUVZeXZ0S1Rxc3ptNDQzT2VNaHgrVEVtclZQS2U5TmF4cEpmLzl5Uk9CV2N3?=
 =?utf-8?B?V3AwbXNXMXg2a1FoVlJZZWdqTVgwdnNqbzA5MWljSzhpT215eHhLdjdtRGYw?=
 =?utf-8?B?MEt2TzBvU1VkLzdYSnFIaVRVUlp5YThQTjREUjkwTWJ0V2lWdGNFdzhpcVln?=
 =?utf-8?B?YjZZZkswTHBISzBxVCtjVnFqQnFlZ21FODhQT21acnJiU1M1T2JmcXlDMmcy?=
 =?utf-8?B?VmxwOXQvSHFrYktJSGxIWGpONkU5RXBlN0RGV2dVZXJmWUVYZVJDSGw1VERh?=
 =?utf-8?B?K1dQRnVURWVYSGFvM1VIRmhjZGlRUWQ0TU0rQ0FoTlU4UHRBUzF1V04zSysv?=
 =?utf-8?B?NTZtK2doS25LZDdDcGN2Uzd2eCtqcWdTNzRYQ0xnb3lYUHR0anZyOFpTWVRq?=
 =?utf-8?B?VC9ZZDJtWUtNanZEN0hlc0FibWRsU2NlbnY4K1BSUGxlWnAwZEJpa2cwT1J0?=
 =?utf-8?B?WXYzWHIrRHFLMGdrOTBzQmRvQnplcmRLTWVPOUNISlpMTXl3dSs2WlZhUjVt?=
 =?utf-8?B?WVhGOUZpOUUwSmVPYU0wTjV5WXJaZVFGOXR0bDdsV2Z5NjBFKytraVVvSzlK?=
 =?utf-8?B?RmxIcGl2Z1BlYXlpd2pUSGVQQlU3Ly9tZHBlZXFiWVcxbmNReHY2azN0Q0hM?=
 =?utf-8?B?L2c9PQ==?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f5ce5374-9a31-4551-9e6a-08d9fd49f392
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2022 19:13:51.2579
 (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: gcadmoCIE/rIRAPeIdropny863rvh884vdtehX/B72hxAIgjX0WDlS7dP1JOFImr
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB3016
X-Proofpoint-ORIG-GUID: GYYGdqFjc6Qi1yWBhIcKg7saDg6nxRv4
X-Proofpoint-GUID: GYYGdqFjc6Qi1yWBhIcKg7saDg6nxRv4
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.64.514
 definitions=2022-03-03_09,2022-02-26_01,2022-02-23_01
X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 phishscore=0
 clxscore=1015 malwarescore=0 bulkscore=0 lowpriorityscore=0
 priorityscore=1501 mlxlogscore=999 suspectscore=0 mlxscore=0
 impostorscore=0 adultscore=0 spamscore=0 classifier=spam adjust=0
 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2203030087
X-FB-Internal: deliver
X-Spam-Status: No, score=-3.2 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_LOW,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 3/3/22 10:56 AM, Hao Luo wrote:
> On Wed, Mar 2, 2022 at 12:55 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 2/25/22 3:43 PM, Hao Luo wrote:
>>> This patch allows bpf_syscall prog to perform some basic filesystem
>>> operations: create, remove directories and unlink files. Three bpf
>>> helpers are added for this purpose. When combined with the following
>>> patches that allow pinning and getting bpf objects from bpf prog,
>>> this feature can be used to create directory hierarchy in bpffs that
>>> help manage bpf objects purely using bpf progs.
>>>
>>> The added helpers subject to the same permission checks as their syscall
>>> version. For example, one can not write to a read-only file system;
>>> The identity of the current process is checked to see whether it has
>>> sufficient permission to perform the operations.
>>>
>>> Only directories and files in bpffs can be created or removed by these
>>> helpers. But it won't be too hard to allow these helpers to operate
>>> on files in other filesystems, if we want.
>>>
>>> Signed-off-by: Hao Luo <haoluo@google.com>
>>> ---
>>>    include/linux/bpf.h            |   1 +
>>>    include/uapi/linux/bpf.h       |  26 +++++
>>>    kernel/bpf/inode.c             |   9 +-
>>>    kernel/bpf/syscall.c           | 177 +++++++++++++++++++++++++++++++++
>>>    tools/include/uapi/linux/bpf.h |  26 +++++
>>>    5 files changed, 236 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index f19abc59b6cd..fce5e26179f5 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -1584,6 +1584,7 @@ int bpf_link_new_fd(struct bpf_link *link);
>>>    struct file *bpf_link_new_file(struct bpf_link *link, int *reserved_fd);
>>>    struct bpf_link *bpf_link_get_from_fd(u32 ufd);
>>>
>>> +bool bpf_path_is_bpf_dir(const struct path *path);
>>>    int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
>>>    int bpf_obj_get_user(const char __user *pathname, int flags);
>>>
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index afe3d0d7f5f2..a5dbc794403d 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -5086,6 +5086,29 @@ union bpf_attr {
>>>     *  Return
>>>     *          0 on success, or a negative error in case of failure. On error
>>>     *          *dst* buffer is zeroed out.
>>> + *
>>> + * long bpf_mkdir(const char *pathname, int pathname_sz, u32 mode)
>>
>> Can we make pathname_sz to be u32 instead of int? pathname_sz should
>> never be negative any way.
>>
>> Also, I think it is a good idea to add 'u64 flags' parameter for all
>> three helpers, so we have room in the future to tune for new use cases.
>>
> 
> SG. Will make this change.
> 
> Actually, I think I need to cap patthname_sz from above, to ensure
> pathname_sz isn't too big. Is that necessary? I see there are other
> helpers that don't have this type of check.

There is no need. The verifier should ensure the memory held by pathname 
will have at least size of pathname_sz.

> 
>>> + *   Description
>>> + *           Attempts to create a directory name *pathname*. The argument
>>> + *           *pathname_sz* specifies the length of the string *pathname*.
>>> + *           The argument *mode* specifies the mode for the new directory. It
>>> + *           is modified by the process's umask. It has the same semantic as
>>> + *           the syscall mkdir(2).
>>> + *   Return
>>> + *           0 on success, or a negative error in case of failure.
>>> + *
>>> + * long bpf_rmdir(const char *pathname, int pathname_sz)
>>> + *   Description
>>> + *           Deletes a directory, which must be empty.
>>> + *   Return
>>> + *           0 on sucess, or a negative error in case of failure.
>>> + *
>>> + * long bpf_unlink(const char *pathname, int pathname_sz)
>>> + *   Description
>>> + *           Deletes a name and possibly the file it refers to. It has the
>>> + *           same semantic as the syscall unlink(2).
>>> + *   Return
>>> + *           0 on success, or a negative error in case of failure.
>>>     */
>>>    #define __BPF_FUNC_MAPPER(FN)               \
>>>        FN(unspec),                     \
>>> @@ -5280,6 +5303,9 @@ union bpf_attr {
>>>        FN(xdp_load_bytes),             \
>>>        FN(xdp_store_bytes),            \
>>>        FN(copy_from_user_task),        \
>>> +     FN(mkdir),                      \
>>> +     FN(rmdir),                      \
>>> +     FN(unlink),                     \
>>>        /* */
>>>
>>>    /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>> [...]