* The ubufs->refcount maybe be subtracted twice when tun_get_user failed
@ 2016-11-29 9:30 wangyunjian
2016-11-29 10:06 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: wangyunjian @ 2016-11-29 9:30 UTC (permalink / raw)
To: mst@redhat.com, Jason Wang, netdev@vger.kernel.org; +Cc: caihe
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.
}
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;
}
...
}
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: The ubufs->refcount maybe be subtracted twice when tun_get_user failed 2016-11-29 9:30 The ubufs->refcount maybe be subtracted twice when tun_get_user failed wangyunjian @ 2016-11-29 10:06 ` Jason Wang 2016-11-29 13:27 ` wangyunjian 0 siblings, 1 reply; 5+ messages in thread From: Jason Wang @ 2016-11-29 10:06 UTC (permalink / raw) To: wangyunjian, mst@redhat.com, netdev@vger.kernel.org; +Cc: caihe 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; > } > ... > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: The ubufs->refcount maybe be subtracted twice when tun_get_user failed 2016-11-29 10:06 ` Jason Wang @ 2016-11-29 13:27 ` wangyunjian 2016-11-30 2:53 ` Jason Wang 0 siblings, 1 reply; 5+ messages in thread From: wangyunjian @ 2016-11-29 13:27 UTC (permalink / raw) To: Jason Wang, mst@redhat.com, netdev@vger.kernel.org; +Cc: caihe 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:jasowang@redhat.com] Sent: Tuesday, November 29, 2016 6:07 PM To: wangyunjian; mst@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; > } > ... > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: The ubufs->refcount maybe be subtracted twice when tun_get_user failed 2016-11-29 13:27 ` wangyunjian @ 2016-11-30 2:53 ` Jason Wang 2016-11-30 5:24 ` Jason Wang 0 siblings, 1 reply; 5+ messages in thread From: Jason Wang @ 2016-11-30 2:53 UTC (permalink / raw) To: wangyunjian, mst@redhat.com, netdev@vger.kernel.org; +Cc: caihe On 2016年11月29日 21:27, wangyunjian wrote: > 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: I get your meaning. Thanks for the reporting. Will post patches (since macvtap has similar issue). > > 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----- ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: The ubufs->refcount maybe be subtracted twice when tun_get_user failed 2016-11-30 2:53 ` Jason Wang @ 2016-11-30 5:24 ` Jason Wang 0 siblings, 0 replies; 5+ messages in thread From: Jason Wang @ 2016-11-30 5:24 UTC (permalink / raw) To: wangyunjian, mst@redhat.com, netdev@vger.kernel.org; +Cc: caihe On 2016年11月30日 10:53, Jason Wang wrote: > > > On 2016年11月29日 21:27, wangyunjian wrote: >> 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: > > I get your meaning. Thanks for the reporting. Will post patches (since > macvtap has similar issue). Posted, appreciate if you can have a test on them. Thanks ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-30 5:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-29 9:30 The ubufs->refcount maybe be subtracted twice when tun_get_user failed wangyunjian 2016-11-29 10:06 ` Jason Wang 2016-11-29 13:27 ` wangyunjian 2016-11-30 2:53 ` Jason Wang 2016-11-30 5:24 ` Jason Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).