* 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).