Re: [PATCH net-next v10 14/15] quic: add packet builder base

On Tue, Mar 3, 2026 at 4:19=E2=80=AFAM Paolo Abeni <[email protected]> wrot=
e:
>
> On 2/25/26 3:34 AM, Xin Long wrote:
> > +/* Transmit a QUIC packet, possibly encrypting and bundling it. */
> > +int quic_packet_xmit(struct sock *sk, struct sk_buff *skb)
> > +{
> > +     struct quic_packet *packet =3D quic_packet(sk);
> > +     struct quic_skb_cb *cb =3D QUIC_SKB_CB(skb);
> > +     struct net *net =3D sock_net(sk);
> > +     int err;
> > +
> > +     /* Skip encryption if taglen =3D=3D 0 (e.g., disable_1rtt_encrypt=
ion). */
> > +     if (!packet->taglen[quic_hdr(skb)->form])
> > +             goto xmit;
> > +
> > +     cb->crypto_done =3D quic_packet_encrypt_done;
> > +     /* Associate skb with sk to ensure sk is valid during async encry=
ption completion. */
> > +     WARN_ON(!skb_set_owner_sk_safe(skb, sk));
>
> This is the TX path, how can sk refcout be 0 here? Possibly use
> skb_set_owner_r() directly? At least use the WARN_ON_ONCE() variant and
> add a comment documenting why is needed,
>
skb_set_owner_r() will do memory account with the skb->truesize, which
is not what it wants here. skb_set_owner_sk_safe() is used to keep the
sk not released during async encryption completion, as the comment
above says.

I don't see another set_owner_sk helper for this. Please let me know if
you have a better way for this.

For now I will change from WARN_ON() to WARN_ON_ONCE(), but still keep
skb_set_owner_sk_safe().

> > +     err =3D quic_crypto_encrypt(quic_crypto(sk, packet->level), skb);
> > +     if (err) {
> > +             if (err !=3D -EINPROGRESS) {
> > +                     QUIC_INC_STATS(net, QUIC_MIB_PKT_ENCDROP);
> > +                     kfree_skb(skb);
> > +                     return err;
> > +             }
> > +             QUIC_INC_STATS(net, QUIC_MIB_PKT_ENCBACKLOGS);
> > +             return err;
> > +     }
> > +     if (!cb->resume) /* Encryption completes synchronously. */
> > +             QUIC_INC_STATS(net, QUIC_MIB_PKT_ENCFASTPATHS);
> > +
> > +xmit:
> > +     if (quic_packet_bundle(sk, skb))
> > +             quic_packet_flush(sk);
> > +     return 0;
> > +}
> > +
> > +/* Create and transmit a new QUIC packet. */
> > +int quic_packet_create_and_xmit(struct sock *sk)
> > +{
> > +     struct quic_packet *packet =3D quic_packet(sk);
> > +     struct sk_buff *skb;
> > +     int err;
> > +
> > +     err =3D quic_packet_number_check(sk);
> > +     if (err)
> > +             goto err;
> > +
> > +     if (packet->level)
> > +             skb =3D quic_packet_handshake_create(sk);
> > +     else
> > +             skb =3D quic_packet_app_create(sk);
> > +     if (!skb) {
> > +             err =3D -ENOMEM;
> > +             goto err;
> > +     }
> > +
> > +     err =3D quic_packet_xmit(sk, skb);
> > +     if (err && err !=3D -EINPROGRESS)
> > +             goto err;
> > +
> > +     /* Return 1 if at least one ACK-eliciting (non-PING) frame was se=
nt. */
> > +     return !!packet->frames;
> > +err:
> > +     pr_debug("%s: err: %d\n", __func__, err);
> > +     return 0;
> > +}
> > +
> > +/* Flush any coalesced/bundled QUIC packets. */
> > +void quic_packet_flush(struct sock *sk)
> > +{
> > +     struct quic_path_group *paths =3D quic_paths(sk);
> > +     struct quic_packet *packet =3D quic_packet(sk);
> > +
> > +     if (packet->head) {
> > +             quic_lower_xmit(sk, packet->head,
> > +                             quic_path_daddr(paths, packet->path), &pa=
ths->fl);
> > +             packet->head =3D NULL;
> > +     }
> > +}
> > +
> > +void quic_packet_init(struct sock *sk)
> > +{
> > +     struct quic_packet *packet =3D quic_packet(sk);
> > +
> > +     INIT_LIST_HEAD(&packet->frame_list);
> > +     packet->taglen[0] =3D QUIC_TAG_LEN;
> > +     packet->taglen[1] =3D QUIC_TAG_LEN;
> > +     packet->mss[0] =3D QUIC_MIN_UDP_PAYLOAD;
> > +     packet->mss[1] =3D QUIC_MIN_UDP_PAYLOAD;
>
> The magic number above looks quite obscure, and AFAICS looking at struct
> quick_packet comments have different meaning. Please use some macro inste=
ad.
>
Sure.