From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [PATCH bpf 7/7] bpf: make direct packet write unclone more robust
Date: Thu, 25 Oct 2018 00:08:05 +0200
Message-ID: <a851d791-8477-75aa-f0ca-0400c841f987@iogearbox.net>
References: <20181024200549.8516-1-daniel@iogearbox.net>
 <20181024200549.8516-8-daniel@iogearbox.net>
 <CAPhsuW4V46Hi=q_fSma3mgji_JQxhjjpa1hPEtEGjE_WvEamhg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: Alexei Starovoitov <ast@kernel.org>,
        Networking <netdev@vger.kernel.org>
To: Song Liu <liu.song.a23@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from www62.your-server.de ([213.133.104.62]:45470 "EHLO
        www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1725829AbeJYGh6 (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 25 Oct 2018 02:37:58 -0400
In-Reply-To: <CAPhsuW4V46Hi=q_fSma3mgji_JQxhjjpa1hPEtEGjE_WvEamhg@mail.gmail.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 10/24/2018 11:42 PM, Song Liu wrote:
> On Wed, Oct 24, 2018 at 1:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Given this seems to be quite fragile and can easily slip through the
>> cracks, lets make direct packet write more robust by requiring that
>> future program types which allow for such write must provide a prologue
>> callback. In case of XDP and sk_msg it's noop, thus add a generic noop
>> handler there. The latter starts out with NULL data/data_end unconditionally
>> when sg pages are shared.
>>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> Acked-by: Alexei Starovoitov <ast@kernel.org>
>> ---
>>  kernel/bpf/verifier.c |  6 +++++-
>>  net/core/filter.c     | 11 +++++++++++
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 5fc9a65..171a2c8 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -5709,7 +5709,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>>         bool is_narrower_load;
>>         u32 target_size;
>>
>> -       if (ops->gen_prologue) {
>> +       if (ops->gen_prologue || env->seen_direct_write) {
>> +               if (!ops->gen_prologue) {
>> +                       verbose(env, "bpf verifier is misconfigured\n");
>> +                       return -EINVAL;
>> +               }
> 
> nit: how about this?
> 
> diff --git i/kernel/bpf/verifier.c w/kernel/bpf/verifier.c
> index 6fbe7a8afed7..d35078024e35 100644
> --- i/kernel/bpf/verifier.c
> +++ w/kernel/bpf/verifier.c
> @@ -5286,6 +5286,11 @@ static int convert_ctx_accesses(struct
> bpf_verifier_env *env)
>         bool is_narrower_load;
>         u32 target_size;
> 
> +       if (!ops->gen_prologue && env->seen_direct_write) {
> +               verbose(env, "bpf verifier is misconfigured\n");
> +               return -EINVAL;
> +       }
> +
>         if (ops->gen_prologue) {
>                 cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
>                                         env->prog);
> 

Hm, probably matter of different style preference, personally I'd prefer
the one as is though.

Thanks,
Daniel