On Tue, Jan 31, 2023 at 5:00 AM Jesper Dangaard Brouer <jbrouer@xxxxxxxxxx> wrote: > > > > On 27/01/2023 18.18, Stanislav Fomichev wrote: > > On Fri, Jan 27, 2023 at 5:58 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > >> > >> Jesper Dangaard Brouer <brouer@xxxxxxxxxx> writes: > >> > >>> The AF_XDP userspace part of xdp_hw_metadata see non-zero as a signal of > >>> the availability of rx_timestamp and rx_hash in data_meta area. The > >>> kernel-side BPF-prog code doesn't initialize these members when kernel > >>> returns an error e.g. -EOPNOTSUPP. This memory area is not guaranteed to > >>> be zeroed, and can contain garbage/previous values, which will be read > >>> and interpreted by AF_XDP userspace side. > >>> > >>> Tested this on different drivers. The experiences are that for most > >>> packets they will have zeroed this data_meta area, but occasionally it > >>> will contain garbage data. > >>> > >>> Example of failure tested on ixgbe: > >>> poll: 1 (0) > >>> xsk_ring_cons__peek: 1 > >>> 0x18ec788: rx_desc[0]->addr=100000000008000 addr=8100 comp_addr=8000 > >>> rx_hash: 3697961069 > >>> rx_timestamp: 9024981991734834796 (sec:9024981991.7348) > >>> 0x18ec788: complete idx=8 addr=8000 > >>> > >>> Converting to date: > >>> date -d @9024981991 > >>> 2255-12-28T20:26:31 CET > >>> > >>> I choose a simple fix in this patch. When kfunc fails or isn't supported > >>> assign zero to the corresponding struct meta value. > >>> > >>> It's up to the individual BPF-programmer to do something smarter e.g. > >>> that fits their use-case, like getting a software timestamp and marking > >>> a flag that gives the type of timestamp. > >>> > >>> Another possibility is for the behavior of kfunc's > >>> bpf_xdp_metadata_rx_timestamp and bpf_xdp_metadata_rx_hash to require > >>> clearing return value pointer. > >> > >> I definitely think we should leave it up to the BPF programmer to react > >> to failures; that's what the return code is there for, after all :) > > > > +1 > > +1 I agree. > We should keep this default functions as simple as possible, for future > "unroll" of BPF-bytecode. > > I the -EOPNOTSUPP case (default functions for drivers not implementing > kfunc), will likely be used runtime by BPF-prog to determine if the > hardware have this offload hint, but it comes with the overhead of a > function pointer call. > > I hope we can somehow BPF-bytecode "unroll" these (default functions) at > BPF-load time, to remove this overhead, and perhaps even let BPF > bytecode do const propagation and code elimination? > > > > Maybe we can unconditionally memset(meta, sizeof(*meta), 0) in > > tools/testing/selftests/bpf/progs/xdp_hw_metadata.c? > > Since it's not a performance tool, it should be ok functionality-wise. > > I know this isn't a performance test, but IMHO always memsetting > metadata area is a misleading example. We know from experience that > developer simply copy-paste code examples, even quick-n-dirty testing > example code. > > The specific issue in this example can lead to hard-to-find bugs, as my > testing shows it is only occasionally that data_meta area contains > garbage. We could do a memset, but it deserves a large code comment, why > this is needed, so people copy-pasting understand. I choose current > approach to keep code close to code people will copy-paste. SG, I don't think it matters, but agreed that having this stated explicitly could help with a blind copy-paste :-) Then maybe repost with the TODO's removed from the kfucs? We seem to agree that it's the user's job to manage the final buffer.. > --Jesper >
