Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190627202417.33370-1-brianvv@google.com> <20190627202417.33370-3-brianvv@google.com>
 <20190627221253.fjsa2lzog2zs5nyz@ast-mbp.dhcp.thefacebook.com>
In-Reply-To: <20190627221253.fjsa2lzog2zs5nyz@ast-mbp.dhcp.thefacebook.com>
From:   Brian Vazquez <brianvv.kernel@gmail.com>
Date:   Fri, 28 Jun 2019 10:49:05 -0700
Message-ID: <CABCgpaWZKac+yAoohUbR0otv4EjBdj9ogA-oZJ2=mHkmqb8AVQ@mail.gmail.com>
Subject: Re: [RFC PATCH bpf-next v2 2/6] bpf: add BPF_MAP_DUMP command to
 access more than one entry per call
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     Brian Vazquez <brianvv@google.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        "David S . Miller" <davem@davemloft.net>,
        Stanislav Fomichev <sdf@google.com>,
        Willem de Bruijn <willemb@google.com>,
        Petar Penkov <ppenkov@google.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> Please explain the api behavior and corner cases in the commit log
> or in code comments.

Ack, will prepare a new version adding those.

> Would it make sense to return last key back into prev_key,
> so that next map_dump command doesn't need to copy it from the
> buffer?

Actually that's a good idea.


> checkpatch.pl please.

 I did use the script and it didn't complain, are you seeing something?

> > +next:
> > +             if (signal_pending(current)) {
> > +                     err = -EINTR;
> > +                     break;
> > +             }
> > +
> > +             rcu_read_lock();
> > +             err = map->ops->map_get_next_key(map, prev_key, key);
> > +             rcu_read_unlock();
> > +
> > +             if (err)
> > +                     break;
>
> should probably be only for ENOENT case?

Yes, this makes sense.

> and other errors should be returned to user ?

and what if the error happened when we had already copied some
entries? Current behavior masks the error to 0 if we copied at least 1
element

>
> > +
> > +             if (bpf_map_copy_value(map, key, value, attr->dump.flags))
> > +                     goto next;
>
> only for ENOENT as well?
> and instead of goto use continue and move cp_len+= to the end after prev_key=key?

Right

>
> > +
> > +             if (copy_to_user(ubuf + cp_len, buf, elem_size))
> > +                     break;
>
> return error to user?
>
> > +
> > +             prev_key = key;
> > +     }
> > +
> > +     if (cp_len)
> > +             err = 0;
>
> this will mask any above errors if there was at least one element copied.

So in general if we copied elements and suddenly we find and error we
should return that error and maybe set cp_len to 0 to 'invalidate' the
data that was already copied?
Yes, I think that sounds like the correct thing to do, what do you think?