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;
}
...
}
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;
}
...
}
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;
}
...
}
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-----
On 2016年11月30日 10:53, Jason Wang wrote:
On 2016年11月29日 21:27, wangyunjian wrote:
quoted
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