Re: [PATCH] net/packet: support vhost mrg_rxbuf

[Jianfeng Tan <jianfeng.tan@linux.alibaba.com>]

On 10/29/2018 10:54 AM, Jason Wang wrote:
>
> On 2018/10/27 下午8:04, Jianfeng Tan wrote:
>> Previouly, virtio net header size is hardcoded to be 10, which makes
>> the feature mrg_rxbuf not available.
>>
>> We redefine PACKET_VNET_HDR ioctl which treats user input as boolean,
>> but now as int, 0, 10, 12, or everything else be treated as 10.
>>
>> There will be one case which is treated differently: if user input is
>> 12, previously, the header size will be 10; but now it's 12.
>>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@linux.alibaba.com>
>
>
> This should go for net-next which is closed. You may consider to 
> re-submit when it was open.

Thank you for the reminder. We'll re-evaluate the necessity of this patch.

>
>
>> ---
>>   net/packet/af_packet.c | 97 ++++++++++++++++++++++++++----------------
>>   net/packet/diag.c      |  2 +-
>>   net/packet/internal.h  |  2 +-
>>   3 files changed, 63 insertions(+), 38 deletions(-)
>>
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index ec3095f13aae..1bd7f4cdcc80 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -1999,18 +1999,24 @@ static unsigned int run_filter(struct sk_buff 
>> *skb,
>>   }
>>     static int packet_rcv_vnet(struct msghdr *msg, const struct 
>> sk_buff *skb,
>> -               size_t *len)
>> +               size_t *len, int vnet_hdr_len)
>>   {
>> +    int res;
>>       struct virtio_net_hdr vnet_hdr;
>>   -    if (*len < sizeof(vnet_hdr))
>> +    if (*len < vnet_hdr_len)
>>           return -EINVAL;
>> -    *len -= sizeof(vnet_hdr);
>> +    *len -= vnet_hdr_len;
>>         if (virtio_net_hdr_from_skb(skb, &vnet_hdr, vio_le(), true, 0))
>>           return -EINVAL;
>>   -    return memcpy_to_msg(msg, (void *)&vnet_hdr, sizeof(vnet_hdr));
>> +    res = memcpy_to_msg(msg, (void *)&vnet_hdr, sizeof(vnet_hdr));
>> +    if (res == 0)
>> +        iov_iter_advance(&msg->msg_iter,
>> +                 vnet_hdr_len - sizeof(vnet_hdr));
>> +
>> +    return res;
>>   }
>>     /*
>> @@ -2206,11 +2212,13 @@ static int tpacket_rcv(struct sk_buff *skb, 
>> struct net_device *dev,
>>                     po->tp_reserve;
>>       } else {
>>           unsigned int maclen = skb_network_offset(skb);
>> +        int vnet_hdr_sz = READ_ONCE(po->vnet_hdr_sz);
>> +
>>           netoff = TPACKET_ALIGN(po->tp_hdrlen +
>>                          (maclen < 16 ? 16 : maclen)) +
>>                          po->tp_reserve;
>> -        if (po->has_vnet_hdr) {
>> -            netoff += sizeof(struct virtio_net_hdr);
>> +        if (vnet_hdr_sz) {
>> +            netoff += vnet_hdr_sz;
>>               do_vnet = true;
>>           }
>>           macoff = netoff - maclen;
>> @@ -2429,19 +2437,6 @@ static int __packet_snd_vnet_parse(struct 
>> virtio_net_hdr *vnet_hdr, size_t len)
>>       return 0;
>>   }
>>   -static int packet_snd_vnet_parse(struct msghdr *msg, size_t *len,
>> -                 struct virtio_net_hdr *vnet_hdr)
>> -{
>> -    if (*len < sizeof(*vnet_hdr))
>> -        return -EINVAL;
>> -    *len -= sizeof(*vnet_hdr);
>> -
>> -    if (!copy_from_iter_full(vnet_hdr, sizeof(*vnet_hdr), 
>> &msg->msg_iter))
>> -        return -EFAULT;
>> -
>> -    return __packet_snd_vnet_parse(vnet_hdr, *len);
>> -}
>> -
>>   static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff 
>> *skb,
>>           void *frame, struct net_device *dev, void *data, int tp_len,
>>           __be16 proto, unsigned char *addr, int hlen, int copylen,
>> @@ -2609,6 +2604,7 @@ static int tpacket_snd(struct packet_sock *po, 
>> struct msghdr *msg)
>>       int len_sum = 0;
>>       int status = TP_STATUS_AVAILABLE;
>>       int hlen, tlen, copylen = 0;
>> +    int vnet_hdr_sz;
>>         mutex_lock(&po->pg_vec_lock);
>>   @@ -2648,7 +2644,8 @@ static int tpacket_snd(struct packet_sock 
>> *po, struct msghdr *msg)
>>       size_max = po->tx_ring.frame_size
>>           - (po->tp_hdrlen - sizeof(struct sockaddr_ll));
>>   -    if ((size_max > dev->mtu + reserve + VLAN_HLEN) && 
>> !po->has_vnet_hdr)
>> +    vnet_hdr_sz = READ_ONCE(po->vnet_hdr_sz);
>> +    if ((size_max > dev->mtu + reserve + VLAN_HLEN) && !vnet_hdr_sz)
>>           size_max = dev->mtu + reserve + VLAN_HLEN;
>>         do {
>> @@ -2668,10 +2665,10 @@ static int tpacket_snd(struct packet_sock 
>> *po, struct msghdr *msg)
>>           status = TP_STATUS_SEND_REQUEST;
>>           hlen = LL_RESERVED_SPACE(dev);
>>           tlen = dev->needed_tailroom;
>> -        if (po->has_vnet_hdr) {
>> +        if (vnet_hdr_sz) {
>>               vnet_hdr = data;
>> -            data += sizeof(*vnet_hdr);
>> -            tp_len -= sizeof(*vnet_hdr);
>> +            data += vnet_hdr_sz;
>> +            tp_len -= vnet_hdr_sz;
>>               if (tp_len < 0 ||
>>                   __packet_snd_vnet_parse(vnet_hdr, tp_len)) {
>>                   tp_len = -EINVAL;
>> @@ -2696,7 +2693,7 @@ static int tpacket_snd(struct packet_sock *po, 
>> struct msghdr *msg)
>>                         addr, hlen, copylen, &sockc);
>>           if (likely(tp_len >= 0) &&
>>               tp_len > dev->mtu + reserve &&
>> -            !po->has_vnet_hdr &&
>> +            !vnet_hdr_sz &&
>>               !packet_extra_vlan_len_allowed(dev, skb))
>>               tp_len = -EMSGSIZE;
>>   @@ -2715,7 +2712,7 @@ static int tpacket_snd(struct packet_sock 
>> *po, struct msghdr *msg)
>>               }
>>           }
>>   -        if (po->has_vnet_hdr) {
>> +        if (vnet_hdr_sz) {
>>               if (virtio_net_hdr_to_skb(skb, vnet_hdr, vio_le())) {
>>                   tp_len = -EINVAL;
>>                   goto tpacket_error;
>> @@ -2802,9 +2799,9 @@ static int packet_snd(struct socket *sock, 
>> struct msghdr *msg, size_t len)
>>       int err, reserve = 0;
>>       struct sockcm_cookie sockc;
>>       struct virtio_net_hdr vnet_hdr = { 0 };
>> +    int vnet_hdr_sz;
>>       int offset = 0;
>>       struct packet_sock *po = pkt_sk(sk);
>> -    bool has_vnet_hdr = false;
>>       int hlen, tlen, linear;
>>       int extra_len = 0;
>>   @@ -2844,11 +2841,29 @@ static int packet_snd(struct socket *sock, 
>> struct msghdr *msg, size_t len)
>>         if (sock->type == SOCK_RAW)
>>           reserve = dev->hard_header_len;
>> -    if (po->has_vnet_hdr) {
>> -        err = packet_snd_vnet_parse(msg, &len, &vnet_hdr);
>> -        if (err)
>> +
>> +    vnet_hdr_sz = READ_ONCE(po->vnet_hdr_sz);
>> +    if (vnet_hdr_sz) {
>> +        if (len < vnet_hdr_sz) {
>> +            err = -EINVAL;
>>               goto out_unlock;
>> -        has_vnet_hdr = true;
>> +        }
>> +        len -= vnet_hdr_sz;
>> +
>> +        if (!copy_from_iter_full(&vnet_hdr, sizeof(vnet_hdr),
>> +                     &msg->msg_iter)) {
>> +            err = -EFAULT;
>> +            goto out_unlock;
>> +        }
>> +
>> +        if (__packet_snd_vnet_parse(&vnet_hdr, len)) {
>> +            err = -EINVAL;
>> +            goto out_unlock;
>> +        }
>
>
> Any reason to open code packet_snd_vnet_parse() here?

No particular reason. Will try to add an parameter, and keep the vnet 
related code inside that function if there will be resubmit.

>
>
>> +
>> +        /* TODO: check hdr_len with len? */
>> +
>> +        iov_iter_advance(&msg->msg_iter, vnet_hdr_sz - 
>> sizeof(vnet_hdr));
>>       }
>>         if (unlikely(sock_flag(sk, SOCK_NOFCS))) {
>> @@ -2912,7 +2927,7 @@ static int packet_snd(struct socket *sock, 
>> struct msghdr *msg, size_t len)
>>       skb->mark = sockc.mark;
>>       skb->tstamp = sockc.transmit_time;
>>   -    if (has_vnet_hdr) {
>> +    if (vnet_hdr_sz) {
>>           err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
>>           if (err)
>>               goto out_free;
>> @@ -3307,11 +3322,11 @@ static int packet_recvmsg(struct socket 
>> *sock, struct msghdr *msg, size_t len,
>>       if (pkt_sk(sk)->pressure)
>>           packet_rcv_has_room(pkt_sk(sk), NULL);
>>   -    if (pkt_sk(sk)->has_vnet_hdr) {
>> -        err = packet_rcv_vnet(msg, skb, &len);
>> +    vnet_hdr_len = READ_ONCE(pkt_sk(sk)->vnet_hdr_sz);
>> +    if (vnet_hdr_len) {
>> +        err = packet_rcv_vnet(msg, skb, &len, vnet_hdr_len);
>>           if (err)
>>               goto out_free;
>> -        vnet_hdr_len = sizeof(struct virtio_net_hdr);
>>       }
>>         /* You lose any data beyond the buffer you gave. If it worries
>> @@ -3772,7 +3787,17 @@ packet_setsockopt(struct socket *sock, int 
>> level, int optname, char __user *optv
>>           if (po->rx_ring.pg_vec || po->tx_ring.pg_vec) {
>>               ret = -EBUSY;
>>           } else {
>> -            po->has_vnet_hdr = !!val;
>> +            /* Previouly we treat user input as boolean (!!val),
>> +             * now we treat it as int. After the below correction,
>> +             * the only violation case is 12, which results in
>> +             * vnet header size of 12 instead of 10.
>> +             */
>> +            if (val &&
>> +                val != sizeof(struct virtio_net_hdr) &&
>> +                val != sizeof(struct virtio_net_hdr_mrg_rxbuf))
>> +                val = sizeof(struct virtio_net_hdr);
>> +
>> +            po->vnet_hdr_sz = val;
>>               ret = 0;
>>           }
>>           release_sock(sk);
>> @@ -3903,7 +3928,7 @@ static int packet_getsockopt(struct socket 
>> *sock, int level, int optname,
>>           val = po->origdev;
>>           break;
>>       case PACKET_VNET_HDR:
>> -        val = po->has_vnet_hdr;
>> +        val = po->vnet_hdr_sz;
>
>
> So the change here is noticeable by userspace. Maybe we need a new opt 
> for this?

Nice catch, users may assume that only 0 or 1 is returned.

Thanks,
Jianfeng

>
> Thanks
>
>
>>           break;
>>       case PACKET_VERSION:
>>           val = po->tp_version;
>> diff --git a/net/packet/diag.c b/net/packet/diag.c
>> index 7ef1c881ae74..950015b6704f 100644
>> --- a/net/packet/diag.c
>> +++ b/net/packet/diag.c
>> @@ -26,7 +26,7 @@ static int pdiag_put_info(const struct packet_sock 
>> *po, struct sk_buff *nlskb)
>>           pinfo.pdi_flags |= PDI_AUXDATA;
>>       if (po->origdev)
>>           pinfo.pdi_flags |= PDI_ORIGDEV;
>> -    if (po->has_vnet_hdr)
>> +    if (po->vnet_hdr_sz)
>>           pinfo.pdi_flags |= PDI_VNETHDR;
>>       if (po->tp_loss)
>>           pinfo.pdi_flags |= PDI_LOSS;
>> diff --git a/net/packet/internal.h b/net/packet/internal.h
>> index 3bb7c5fb3bff..11bc75950f28 100644
>> --- a/net/packet/internal.h
>> +++ b/net/packet/internal.h
>> @@ -115,9 +115,9 @@ struct packet_sock {
>>       unsigned int        running;    /* bind_lock must be held */
>>       unsigned int        auxdata:1,    /* writer must hold sock lock */
>>                   origdev:1,
>> -                has_vnet_hdr:1,
>>                   tp_loss:1,
>>                   tp_tx_has_off:1;
>> +    int            vnet_hdr_sz;
>>       int            pressure;
>>       int            ifindex;    /* bound device        */
>>       __be16            num;