Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Song Liu <songliubraving@fb.com>
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
CC:     open list <linux-kernel@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, Networking <netdev@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Kernel Team <Kernel-team@fb.com>,
        john fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@chromium.org>,
        Jesper Dangaard Brouer <brouer@redhat.com>,
        Daniel Xu <dlxu@fb.com>
Subject: Re: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
Thread-Topic: [PATCH bpf-next 2/5] libbpf: support BPF_PROG_TYPE_USER programs
Thread-Index: AQHWZ+C4tQwFkmvi20aNhPcONSrEz6klnl6AgAGMFwCAAZgFAIAAJ1IAgAAaAQCAAA8tgIAAB7wAgAAIKwA=
Date:   Wed, 5 Aug 2020 07:23:31 +0000
Message-ID: <88081DFA-39FF-42BB-98F4-CCDE634A1775@fb.com>
References: <20200801084721.1812607-1-songliubraving@fb.com>
 <20200801084721.1812607-3-songliubraving@fb.com>
 <CAEf4BzYp4gO1P+OrY7hGyQjdia3BuSu4DX2_z=UF6RfGNa+gkQ@mail.gmail.com>
 <9C1285C1-ECD6-46BD-BA95-3E9E81C00EF0@fb.com>
 <CAEf4BzYojfFiMn6VeUkxUsdSTdFK0A4MzKQxhCCp_OowkseznQ@mail.gmail.com>
 <5BC1D7AD-32C1-4CDC-BA99-F4DABE61EEA3@fb.com>
 <CAEf4BzbbCZmijrU4vfkmq2PFsMMFG+xz9qR1e4wfrdm6tF4_hA@mail.gmail.com>
 <4DB698F2-BC51-4E96-BC3B-F478BE9AE106@fb.com>
 <CAEf4Bza4KXkVov=UwouryG5JcqYQ=9mDG8nBoWmb97rv+_yqTw@mail.gmail.com>
In-Reply-To: <CAEf4Bza4KXkVov=UwouryG5JcqYQ=9mDG8nBoWmb97rv+_yqTw@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5664687029CD5C4B9B5A94A62460FF62@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 843dde1c-480b-4211-b035-08d839107534
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Aug 2020 07:23:31.9963
 (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: 5ExoabcYDBAECA+lLBbee/A597vjJxHdnOxb6wcZKajUrrc4L3RJoAF65hPZv2p+S9kKFu4ODhHl1gQOEvlQyg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB2728
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687
 definitions=2020-08-05_05:2020-08-03,2020-08-05 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 malwarescore=0
 phishscore=0 clxscore=1015 adultscore=0 bulkscore=0 mlxlogscore=999
 suspectscore=0 mlxscore=0 spamscore=0 priorityscore=1501
 lowpriorityscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2006250000 definitions=main-2008050062
X-FB-Internal: deliver
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Aug 4, 2020, at 11:54 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> =
wrote:
>=20
> On Tue, Aug 4, 2020 at 11:26 PM Song Liu <songliubraving@fb.com> wrote:
>>=20
>>=20
>>=20
>>> On Aug 4, 2020, at 10:32 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com=
> wrote:
>>>=20
>>> On Tue, Aug 4, 2020 at 8:59 PM Song Liu <songliubraving@fb.com> wrote:
>>>>=20
>>>>=20
>>>>=20
>>>>> On Aug 4, 2020, at 6:38 PM, Andrii Nakryiko <andrii.nakryiko@gmail.co=
m> wrote:
>>>>>=20
>>>>> On Mon, Aug 3, 2020 at 6:18 PM Song Liu <songliubraving@fb.com> wrote=
:
>>>>>>=20
>>>>>>=20
>>>>>>=20
>>>>>>> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko <andrii.nakryiko@gmail.=
com> wrote:
>>>>>>>=20
>>>>>>> On Sat, Aug 1, 2020 at 1:50 AM Song Liu <songliubraving@fb.com> wro=
te:
>>>>>>>>=20
>>>>>>=20
>>>>>> [...]
>>>>>>=20
>>>>>>>=20
>>>>>>>> };
>>>>>>>>=20
>>>>>>>> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_at=
tr *test_attr);
>>>>>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>>>>>> index b9f11f854985b..9ce175a486214 100644
>>>>>>>> --- a/tools/lib/bpf/libbpf.c
>>>>>>>> +++ b/tools/lib/bpf/libbpf.c
>>>>>>>> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs=
[] =3D {
>>>>>>>>     BPF_PROG_SEC("lwt_out",                 BPF_PROG_TYPE_LWT_OUT)=
,
>>>>>>>>     BPF_PROG_SEC("lwt_xmit",                BPF_PROG_TYPE_LWT_XMIT=
),
>>>>>>>>     BPF_PROG_SEC("lwt_seg6local",           BPF_PROG_TYPE_LWT_SEG6=
LOCAL),
>>>>>>>> +       BPF_PROG_SEC("user",                    BPF_PROG_TYPE_USER=
),
>>>>>>>=20
>>>>>>> let's do "user/" for consistency with most other prog types (and ni=
ce
>>>>>>> separation between prog type and custom user name)
>>>>>>=20
>>>>>> About "user" vs. "user/", I still think "user" is better.
>>>>>>=20
>>>>>> Unlike kprobe and tracepoint, user prog doesn't use the part after "=
/".
>>>>>> This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" =
for
>>>>>> BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and "user=
/xxx"
>>>>>> would also work. However, if we specify "user/" here, programs that =
used
>>>>>> "user" by accident will fail to load, with a message like:
>>>>>>=20
>>>>>>      libbpf: failed to load program 'user'
>>>>>>=20
>>>>>> which is confusing.
>>>>>=20
>>>>> xdp, perf_event and a bunch of others don't enforce it, that's true,
>>>>> they are a bit of a legacy,
>>>>=20
>>>> I don't see w/o "/" is a legacy thing. BPF_PROG_TYPE_STRUCT_OPS just u=
ses
>>>> "struct_ops".
>>>>=20
>>>>> unfortunately. But all the recent ones do,
>>>>> and we explicitly did that for xdp_dev/xdp_cpu, for instance.
>>>>> Specifying just "user" in the spec would allow something nonsensical
>>>>> like "userargh", for instance, due to this being treated as a prefix.
>>>>> There is no harm to require users to do "user/my_prog", though.
>>>>=20
>>>> I don't see why allowing "userargh" is a problem. Failing "user" is
>>>> more confusing. We can probably improve that by a hint like:
>>>>=20
>>>>   libbpf: failed to load program 'user', do you mean "user/"?
>>>>=20
>>>> But it is pretty silly. "user/something_never_used" also looks weird.
>>>=20
>>> "userargh" is terrible, IMO. It's a different identifier that just
>>> happens to have the first 4 letters matching "user" program type.
>>> There must be either a standardized separator (which happens to be
>>> '/') or none. See the suggestion below.
>>=20
>> We have no problem deal with "a different identifier that just happens
>> to have the first letters matching", like xdp vs. xdp_devmap and
>> xdp_cpumap, right?
>>=20
>=20
> xdp vs xdp_devmap is an entirely different thing. We deal with it by
> checking xdp_devmap first. What I'm saying is that user can do
> "xdpomg" and libbpf would be happy (today). And I don't think that's
> good. But further, if someone does something like "xdp_devmap_omg",
> guess which program type will be inferred? Hint: not xdp_devmap and
> libbpf won't report an error either. All because "xdp" is so lax
> today.
>=20
>>>>=20
>>>>> Alternatively, we could introduce a new convention in the spec,
>>>>> something like "user?", which would accept either "user" or
>>>>> "user/something", but not "user/" nor "userblah". We can try that as
>>>>> well.
>>>>=20
>>>> Again, I don't really understand why allowing "userblah" is a problem.
>>>> We already have "xdp", "xdp_devmap/", and "xdp_cpumap/", they all work
>>>> fine so far.
>>>=20
>>> Right, we have "xdp_devmap/" and "xdp_cpumap/", as you say. I haven't
>>> seen so much pushback against trailing forward slash with those ;)
>>=20
>> I haven't seen any issue with old "perf_event", "xdp" and new "struct_op=
s"
>> either.
>>=20
>>>=20
>>> But anyways, as part of deprecating APIs and preparing libbpf for 1.0
>>> release over this half, I think I'm going to emit warnings for names
>>> like "prog_type_whatever" or "prog_typeevenworse", etc. And asking
>>> users to normalize section names to either "prog_type" or
>>> "prog_type/something/here", whichever makes sense for a specific
>>> program type.
>>=20
>> Exactly, "user" makes sense here; while "kprobe/__set_task_comm" makes
>> sense for kprobe.
>=20
> Right, but "userblah" doesn't. It would be great if you could help
> make what I described above become true. But at least don't make it
> worse by allowing unrestricted "user" prefix. I'm OK with strict
> "user" or "user/blah", I'm not OK with "userblah", I'm sorry.

If the concern with "userblah" is real and unbearable, so is "xdpblah"
and "perf_eventblah", and so on, and so on.=20

>=20
>>=20
>>> Right now libbpf doesn't allow two separate BPF programs
>>> with the same section name, so enforcing strict "user" is limiting to
>>> users. We are going to lift that restriction pretty soon, though. But
>>> for now, please stick with what we've been doing lately and mark it as
>>> "user/", later we'll allow just "user" as well.
>>=20
>> Since we would allow "user" later, why we have to reject it for now?
>=20
> Because libbpf is dumb in that regard today? And instead of migrating
> users later, I want to prevent users making bad choices right now.

The good choice here is to use "user", IMO. And you are preventing people=20
to use it. If user has to use "user/" for now. They will have to update=20
the programs later, right? If the conclusion is "user/xxx" is the ultimate
goal, I would agree with "user/" for now.=20

> Then relax it, if necessary. Alternatively, we can fix up libbpf logic
> before the USER program type lands.

I don't see why the USER program type need to wait for libbpf fix, as
"xdp", "perf_event", etc. all work well today.=20

>=20
>> Imagine the user just compiled and booted into a new kernel with user
>> program support; and then got the following message:
>>=20
>>        libbpf: failed to load program 'user'
>>=20
>> If I were the user, I would definitely question whether the kernel was
>> correct...
>=20
> That's also bad, and again, we can make libbpf better. I think moving
> forward any non-recognized BPF program type should be reported by
> libbpf as an error. But we can't do it right now, we have to have a
> period in which users will get a chance to update their BPF programs.
> This will have to happen over few libbpf releases at least. So please
> join in on the fun of fixing stuff like this.

I'd love to join the fun. Maybe after user program lands ;)