On 07/25, Simon Horman wrote:
> On Mon, Jul 24, 2023 at 04:59:51PM -0700, Stanislav Fomichev wrote:
>
> ...
>
> Hi Stan,
>
> > diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> > index bf71698a1e82..cf1e11c76339 100644
> > --- a/include/uapi/linux/netdev.h
> > +++ b/include/uapi/linux/netdev.h
> > @@ -37,11 +37,26 @@ enum netdev_xdp_act {
> > NETDEV_XDP_ACT_MASK = 127,
> > };
> >
> > +/**
> > + * enum netdev_xsk_flags
> > + * @NETDEV_XSK_FLAGS_TX_TIMESTAMP: HW timestamping egress packets is supported
> > + * by the driver.
> > + * @NETDEV_XSK_FLAGS_TX_CHECKSUM: L3 checksum HW offload is supported by the
> > + * driver.
> > + */
> > +enum netdev_xsk_flags {
> > + NETDEV_XSK_FLAGS_TX_TIMESTAMP = 1,
> > + NETDEV_XSK_FLAGS_TX_CHECKSUM = 2,
> > +
>
> I know that it isn't the practice in this file.
> but adding the following makes kernel-doc happier
> about NETDEV_XSK_FLAGS_MASK not being documented.
>
> /* private: */
This is autogenerated file :-( But I guess I can try to extend ynl
scripts to put this comment before the mask. Let me look into that...
> > + NETDEV_XSK_FLAGS_MASK = 3,
> > +};
> > +
> > enum {
> > NETDEV_A_DEV_IFINDEX = 1,
> > NETDEV_A_DEV_PAD,
> > NETDEV_A_DEV_XDP_FEATURES,
> > NETDEV_A_DEV_XDP_ZC_MAX_SEGS,
> > + NETDEV_A_DEV_XSK_FEATURES,
> >
> > __NETDEV_A_DEV_MAX,
> > NETDEV_A_DEV_MAX = (__NETDEV_A_DEV_MAX - 1)
>
> ...
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
>
> ...
>
> > @@ -626,6 +635,7 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> > static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > struct xdp_desc *desc)
> > {
> > + struct xsk_tx_metadata *meta = NULL;
> > struct net_device *dev = xs->dev;
> > struct sk_buff *skb = xs->skb;
> > int err;
> > @@ -678,12 +688,40 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> >
> > skb_add_rx_frag(skb, nr_frags, page, 0, len, 0);
> > }
> > +
> > + if (desc->options & XDP_TX_METADATA) {
> > + if (unlikely(xs->tx_metadata_len == 0)) {
> > + err = -EINVAL;
> > + goto free_err;
> > + }
> > +
> > + meta = buffer - xs->tx_metadata_len;
> > +
> > + if (meta->flags & XDP_TX_METADATA_CHECKSUM) {
> > + if (unlikely(meta->csum_start + meta->csum_offset +
> > + sizeof(__sum16) > len)) {
> > + err = -EINVAL;
> > + goto free_err;
> > + }
> > +
> > + skb->csum_start = hr + meta->csum_start;
>
> hr seems to only be set - by earlier, existing code in this function -
> if skb is NULL. Is it always safe to use it here?
>
> Smatch flags hr as being potentially used uninitialised,
> the above is my understanding of why it thinks that is so.
Ugh, good point, I've missed that. This part is new and it's from
multi-frag support. I need to be more careful here and apply metadata
only from the first descriptor in the chain. Thanks!
