RE: The ubufs->refcount maybe be subtracted twice when tun_get_user failed

[wangyunjian]

Sorry, I didn't describe it clearly. In fact, the second subtraction happens in 
the function handle_tx,
when tun_get_user fails and zcopy_used is ture. Fllowing the steps:

static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
                                  void *msg_control, struct iov_iter *from,
                                  int noblock) {
           ...

           if (zerocopy)
                     err = zerocopy_sg_from_iter(skb, from);
           else {
                     err = skb_copy_datagram_from_iter(skb, 0, from, len);
                     if (!err && msg_control) {
                              struct ubuf_info *uarg = msg_control;
                              uarg->callback(uarg, false);                      
 --> step 1, the ubufs->refcount is subtracted frist.
                     }
           }

           if (err) {
                     this_cpu_inc(tun->pcpu_stats->rx_dropped);
                     kfree_skb(skb);
                     return -EFAULT;
           }

           err = virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun));
           if (err) {
                     this_cpu_inc(tun->pcpu_stats->rx_frame_errors);
                     kfree_skb(skb);
                     return -EINVAL;                                         
-->step 2, return err.
           }
}

static void handle_tx(struct vhost_net *net)
{
        ...
        /* TODO: Check specific error and bomb out unless ENOBUFS? */
        err = sock->ops->sendmsg(sock, &msg, len);
        if (unlikely(err < 0)) {
                if (zcopy_used) {
                        vhost_net_ubuf_put(ubufs);                              
          --> step 3, the ubufs->refcount will be subtracted twice, when 
sendmsg execution err.
                        nvq->upend_idx = ((unsigned)nvq->upend_idx - 1)
                                % UIO_MAXIOV;
                }
                vhost_discard_vq_desc(vq, 1);
                break;
        }
        ...
}

-----Original Message-----
From: Jason Wang [mailto:jasow...@redhat.com] 
Sent: Tuesday, November 29, 2016 6:07 PM
To: wangyunjian; m...@redhat.com; netdev@vger.kernel.org
Cc: caihe
Subject: Re: The ubufs->refcount maybe be subtracted twice when tun_get_user 
failed



On 2016年11月29日 17:30, wangyunjian wrote:
> In function tun_get_user , the ubufs->refcount may be subtracted twice, when 
> msg_control is true and zerocopy is false.
>
> About the below code:
>
> static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>                                  void *msg_control, struct iov_iter *from,
>                                  int noblock) {
>           ...
>
>           if (zerocopy)
>                     err = zerocopy_sg_from_iter(skb, from);
>           else {
>                     err = skb_copy_datagram_from_iter(skb, 0, from, len);
>                     if (!err && msg_control) {
>                              struct ubuf_info *uarg = msg_control;
>                              uarg->callback(uarg, false);                     
>   --> the ubufs->refcount is subtracted frist.
>                     }
>           }
>
>           if (err) {
>                     this_cpu_inc(tun->pcpu_stats->rx_dropped);
>                     kfree_skb(skb);
>                     return -EFAULT;
>           }
>
>           err = virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun));
>           if (err) {
>                     this_cpu_inc(tun->pcpu_stats->rx_frame_errors);
>                     kfree_skb(skb);
>                     return -EINVAL;                                   
> -->here, the ubufs->refcount will be subtracted twice, when 
> virtio_net_hdr_to_skb execution err.
>           }

Just make sure I understand the problem. Since we don't set SKBTX_DEV_ZEROCOPY 
here if zerocopy is false, callback won't be even trigged in 
skb_release_data(). So we are in fact safe here?

Thanks

> switch (tun->flags & TUN_TYPE_MASK) {
>           case IFF_TUN:
>                     if (tun->flags & IFF_NO_PI) {
>                              switch (skb->data[0] & 0xf0) {
>                              case 0x40:
>                                       pi.proto = htons(ETH_P_IP);
>                                       break;
>                              case 0x60:
>                                       pi.proto = htons(ETH_P_IPV6);
>                                       break;
>                              default:
>                                       
> this_cpu_inc(tun->pcpu_stats->rx_dropped);
>                                       kfree_skb(skb);
>                                       return -EINVAL;                         
>  --> this will also be subtracted twice.
>                              }
>                     }
>
>                     skb_reset_mac_header(skb);
>                     skb->protocol = pi.proto;
>                     skb->dev = tun->dev;
>                     break;
>           case IFF_TAP:
>                     skb->protocol = eth_type_trans(skb, tun->dev);
>                     break;
>           }
>               ...
> }