On 6/9/21 10:06 AM, Zvi Effron wrote:
On Sat, Jun 5, 2021 at 10:17 PM Yonghong Song <yhs@xxxxxx> wrote:
On 6/4/21 3:02 PM, Zvi Effron wrote:
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -687,6 +687,38 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
return ret;
}
+static int xdp_convert_md_to_buff(struct xdp_buff *xdp, struct xdp_md *xdp_md)
Should the order of parameters be switched to (xdp_md, xdp)?
This will follow the convention of below function xdp_convert_buff_to_md().
The order was done to match the skb versions of these functions, which seem to
have the output format first and the input format second, which is why the
order flips between conversion functions. We're not particular about order, so
we can definitely make it consistent.
But for another function we have
+static void xdp_convert_buff_to_md(struct xdp_buff *xdp, struct xdp_md
*xdp_md)
The input first and the output second. In my opinion, in the same file,
we should keep the same ordering convention.
+{
+ void *data;
+
+ if (!xdp_md)
+ return 0;
+
+ if (xdp_md->egress_ifindex != 0)
+ return -EINVAL;
+
+ if (xdp_md->data > xdp_md->data_end)
+ return -EINVAL;
+
+ xdp->data = xdp->data_meta + xdp_md->data;
+
+ if (xdp_md->ingress_ifindex != 0 || xdp_md->rx_queue_index != 0)
+ return -EINVAL;
It would be good if you did all error checking before doing xdp->data
assignment. Also looks like xdp_md error checking happens here and
bpf_prog_test_run_xdp(). If it is hard to put all error checking
in bpf_prog_test_run_xdp(), at least put "xdp_md->data >
xdp_md->data_end) in bpf_prog_test_run_xdp(), so this function only
checks *_ifindex and rx_queue_index?
bpf_prog_test_run_xdp() was already a large function, which is why this was
turned into a helper. Initially, we tried to have all xdp_md related logic in
the helper, with only the required logic in bpf_prog_test_run_xdp(). Based on
a prior suggestion, we moved one additional check from the helper to
bpf_prog_test_run_xdp() as it simplified the logic. It's not clear to us what
benefit moving the other checks to bpf_prog_test_run_xdp() provides, but it
does reduce the benefit of having the helper function.
At least put "if (xdp_md->data > xdp_md->data_end)" checking in the
bpf_prog_test_run_xdp() as similar fields are already checked there.
It is okay to put *_ifindex/rx_queue_index in this function since you
need to get device for checking and there is no need to get device twice.
@@ -696,36 +728,68 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
u32 repeat = kattr->test.repeat;
struct netdev_rx_queue *rxqueue;
struct xdp_buff xdp = {};
+ struct xdp_md *ctx;
Let us try to maintain reverse christmas tree?
Sure.
u32 retval, duration;
u32 max_data_sz;
void *data;
int ret;
- if (kattr->test.ctx_in || kattr->test.ctx_out)
- return -EINVAL;
+ ctx = bpf_ctx_init(kattr, sizeof(struct xdp_md));
+ if (IS_ERR(ctx))
+ return PTR_ERR(ctx);
+
+ /* There can't be user provided data before the metadata */
+ if (ctx) {
+ if (ctx->data_meta)
+ return -EINVAL;
+ if (ctx->data_end != size)
+ return -EINVAL;
+ if (unlikely((ctx->data & (sizeof(__u32) - 1)) ||
+ ctx->data > 32))
Why 32? Should it be sizeof(struct xdp_md)?
This is not checking the context itself, but the amount of metadata. XDP allows
at most 32 bytes of metadata.
Do we have a macro for this "32"? It would be good if we have one.
Otherwise, some comments will be good.
Previously I am thinking just enforce ctx->data to be sizeof(struct
xdp_md). But think twice, this is a little bit too restricted.
So your current handling is fine.
+ /* Metadata is allocated from the headroom */
+ headroom -= ctx->data;
sizeof(struct xdp_md) should be smaller than headroom
(XDP_PACKET_HEADROOM), so we don't need to a check, but
some comments might be helpful so people looking at the
code doesn't need to double check.
We're not sure what check you're referring to, as there's no check here. This
subtraction is, as the comment says, because the XDP metadata is allocated out
of the XDP headroom, so the headroom size needs to be reduced by the metadata
size.
I am wondering whether we need to check
if (headroom < ctx->data)
return -EINVAL;
headroom -= ctx->data;
We have
headroom = XDP_PACKET_HEADROOM;
ctx->data <= 32
so we should be okay.
