Re: [PATCH net-next 2/2] net: convert (struct ubuf_info)->refcnt to refcount_t
On Sat, Sep 2, 2017 at 5:58 PM, kbuild test robot wrote: > Hi Eric, > > [auto build test WARNING on net-next/master] > > url: > https://github.com/0day-ci/linux/commits/Eric-Dumazet/net-ubuf_info-refcnt-conversion/20170903-043506 > config: i386-randconfig-i1-201736 (attached as .config) > compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > All warnings (new ones prefixed by >>): > >drivers//vhost/net.c: In function 'handle_tx': >>> drivers//vhost/net.c:536:4: warning: passing argument 1 of 'atomic_set' >>> from incompatible pointer type [enabled by default] >atomic_set(&ubuf->refcnt, 1); >^ >In file included from include/linux/atomic.h:4:0, > from arch/x86/include/asm/thread_info.h:53, > from include/linux/thread_info.h:37, > from arch/x86/include/asm/preempt.h:6, > from include/linux/preempt.h:80, > from include/linux/spinlock.h:50, > from include/linux/wait.h:8, > from include/linux/eventfd.h:12, > from drivers//vhost/net.c:10: >arch/x86/include/asm/atomic.h:36:29: note: expected 'struct atomic_t *' > but argument is of type 'struct refcount_t *' > static __always_inline void atomic_set(atomic_t *v, int i) > ^ This is a false positive. This patch [net-next,2/2] net: convert (struct ubuf_info)->refcnt to refcount_t http://patchwork.ozlabs.org/patch/808402/ was superseded by [v2,net-next,2/2] net: convert (struct ubuf_info)->refcnt to refcount_t http://patchwork.ozlabs.org/patch/808477/ which has been merged into net-next as commit c1d1b437816f That patch has the necessary change: - atomic_set(&ubuf->refcnt, 1); + refcount_set(&ubuf->refcnt, 1);
Re: [PATCH net-next 2/2] net: convert (struct ubuf_info)->refcnt to refcount_t
Hi Eric,
[auto build test WARNING on net-next/master]
url:
https://github.com/0day-ci/linux/commits/Eric-Dumazet/net-ubuf_info-refcnt-conversion/20170903-043506
config: i386-randconfig-i1-201736 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers//vhost/net.c: In function 'handle_tx':
>> drivers//vhost/net.c:536:4: warning: passing argument 1 of 'atomic_set' from
>> incompatible pointer type [enabled by default]
atomic_set(&ubuf->refcnt, 1);
^
In file included from include/linux/atomic.h:4:0,
from arch/x86/include/asm/thread_info.h:53,
from include/linux/thread_info.h:37,
from arch/x86/include/asm/preempt.h:6,
from include/linux/preempt.h:80,
from include/linux/spinlock.h:50,
from include/linux/wait.h:8,
from include/linux/eventfd.h:12,
from drivers//vhost/net.c:10:
arch/x86/include/asm/atomic.h:36:29: note: expected 'struct atomic_t *' but
argument is of type 'struct refcount_t *'
static __always_inline void atomic_set(atomic_t *v, int i)
^
vim +/atomic_set +536 drivers//vhost/net.c
0ed005ce0 Jason Wang 2017-01-18 442
3a4d5c94e Michael S. Tsirkin 2010-01-14 443 /* Expects to be always run from
workqueue - which acts as
3a4d5c94e Michael S. Tsirkin 2010-01-14 444 * read-size critical section for
our kind of RCU. */
3a4d5c94e Michael S. Tsirkin 2010-01-14 445 static void handle_tx(struct
vhost_net *net)
3a4d5c94e Michael S. Tsirkin 2010-01-14 446 {
2839400f8 Asias He 2013-04-27 447struct vhost_net_virtqueue *nvq
= &net->vqs[VHOST_NET_VQ_TX];
81f95a558 Michael S. Tsirkin 2013-04-28 448struct vhost_virtqueue *vq =
&nvq->vq;
98a527aac Al Viro2014-12-10 449unsigned out, in;
d5675bd20 Michael S. Tsirkin 2010-06-24 450int head;
3a4d5c94e Michael S. Tsirkin 2010-01-14 451struct msghdr msg = {
3a4d5c94e Michael S. Tsirkin 2010-01-14 452.msg_name = NULL,
3a4d5c94e Michael S. Tsirkin 2010-01-14 453.msg_namelen = 0,
3a4d5c94e Michael S. Tsirkin 2010-01-14 454.msg_control = NULL,
3a4d5c94e Michael S. Tsirkin 2010-01-14 455.msg_controllen = 0,
3a4d5c94e Michael S. Tsirkin 2010-01-14 456.msg_flags =
MSG_DONTWAIT,
3a4d5c94e Michael S. Tsirkin 2010-01-14 457};
3a4d5c94e Michael S. Tsirkin 2010-01-14 458size_t len, total_len = 0;
70181d512 Jason Wang 2013-04-10 459int err;
3a4d5c94e Michael S. Tsirkin 2010-01-14 460size_t hdr_size;
28457ee69 Arnd Bergmann 2010-03-09 461struct socket *sock;
fe729a57c Asias He 2013-05-06 462struct vhost_net_ubuf_ref
*uninitialized_var(ubufs);
cedb9bdce Michael S. Tsirkin 2012-12-06 463bool zcopy, zcopy_used;
28457ee69 Arnd Bergmann 2010-03-09 464
2e26af79b Asias He 2013-05-07 465mutex_lock(&vq->mutex);
2e26af79b Asias He 2013-05-07 466sock = vq->private_data;
3a4d5c94e Michael S. Tsirkin 2010-01-14 467if (!sock)
2e26af79b Asias He 2013-05-07 468goto out;
3a4d5c94e Michael S. Tsirkin 2010-01-14 469
6b1e6cc78 Jason Wang 2016-06-23 470if (!vq_iotlb_prefetch(vq))
6b1e6cc78 Jason Wang 2016-06-23 471goto out;
6b1e6cc78 Jason Wang 2016-06-23 472
8ea8cf89e Michael S. Tsirkin 2011-05-20 473vhost_disable_notify(&net->dev,
vq);
3a4d5c94e Michael S. Tsirkin 2010-01-14 474
81f95a558 Michael S. Tsirkin 2013-04-28 475hdr_size = nvq->vhost_hlen;
2839400f8 Asias He 2013-04-27 476zcopy = nvq->ubufs;
3a4d5c94e Michael S. Tsirkin 2010-01-14 477
3a4d5c94e Michael S. Tsirkin 2010-01-14 478for (;;) {
bab632d69 Michael S. Tsirkin 2011-07-18 479/* Release DMAs done
buffers first */
bab632d69 Michael S. Tsirkin 2011-07-18 480if (zcopy)
eaae8132e Michael S. Tsirkin 2012-11-01 481
vhost_zerocopy_signal_used(net, vq);
bab632d69 Michael S. Tsirkin 2011-07-18 482
f7c6be404 Jason Wang 2013-09-02 483/* If more outstanding
DMAs, queue the work.
f7c6be404 Jason Wang 2013-09-02 484 * Handle upend_idx
wrap around
f7c6be404 Jason Wang 2013-09-02 485 */
0ed005ce0 Jason Wang 2017-01-18 486if
(unlikely(vhost_exceeds_maxpend(net)))
f7c6be404 Jason Wang 2013-09-02 487break;
f7c6be404 Jason Wang 2013-09-02 488
030881372 Jason Wang 2016-03-04 489head =
vhost_net_tx_get_vq_desc(net, vq, vq->iov,
3a4d5c94e Michael S. Tsirkin 2010-01-14 490
ARRAY_SIZE(vq->iov),
030881372 Jason Wang
Re: [PATCH net-next 2/2] net: convert (struct ubuf_info)->refcnt to refcount_t
Hi Eric,
[auto build test ERROR on net-next/master]
url:
https://github.com/0day-ci/linux/commits/Eric-Dumazet/net-ubuf_info-refcnt-conversion/20170903-043506
config: x86_64-acpi-redef (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers//vhost/net.c: In function 'handle_tx':
>> drivers//vhost/net.c:536:15: error: passing argument 1 of 'atomic_set' from
>> incompatible pointer type [-Werror=incompatible-pointer-types]
atomic_set(&ubuf->refcnt, 1);
^
In file included from include/linux/atomic.h:4:0,
from include/linux/jump_label.h:183,
from arch/x86/include/asm/string_64.h:5,
from arch/x86/include/asm/string.h:4,
from include/linux/string.h:18,
from include/linux/bitmap.h:8,
from include/linux/cpumask.h:11,
from arch/x86/include/asm/cpumask.h:4,
from arch/x86/include/asm/msr.h:10,
from arch/x86/include/asm/processor.h:20,
from arch/x86/include/asm/cpufeature.h:4,
from arch/x86/include/asm/thread_info.h:52,
from include/linux/thread_info.h:37,
from arch/x86/include/asm/preempt.h:6,
from include/linux/preempt.h:80,
from include/linux/spinlock.h:50,
from include/linux/seqlock.h:35,
from include/linux/time.h:5,
from include/linux/stat.h:18,
from include/linux/compat.h:12,
from drivers//vhost/net.c:9:
arch/x86/include/asm/atomic.h:36:29: note: expected 'atomic_t * {aka struct
*}' but argument is of type 'refcount_t * {aka struct
refcount_struct *}'
static __always_inline void atomic_set(atomic_t *v, int i)
^~
cc1: some warnings being treated as errors
vim +/atomic_set +536 drivers//vhost/net.c
0ed005ce0 Jason Wang 2017-01-18 442
3a4d5c94e Michael S. Tsirkin 2010-01-14 443 /* Expects to be always run from
workqueue - which acts as
3a4d5c94e Michael S. Tsirkin 2010-01-14 444 * read-size critical section for
our kind of RCU. */
3a4d5c94e Michael S. Tsirkin 2010-01-14 445 static void handle_tx(struct
vhost_net *net)
3a4d5c94e Michael S. Tsirkin 2010-01-14 446 {
2839400f8 Asias He 2013-04-27 447struct vhost_net_virtqueue *nvq
= &net->vqs[VHOST_NET_VQ_TX];
81f95a558 Michael S. Tsirkin 2013-04-28 448struct vhost_virtqueue *vq =
&nvq->vq;
98a527aac Al Viro2014-12-10 449unsigned out, in;
d5675bd20 Michael S. Tsirkin 2010-06-24 450int head;
3a4d5c94e Michael S. Tsirkin 2010-01-14 451struct msghdr msg = {
3a4d5c94e Michael S. Tsirkin 2010-01-14 452.msg_name = NULL,
3a4d5c94e Michael S. Tsirkin 2010-01-14 453.msg_namelen = 0,
3a4d5c94e Michael S. Tsirkin 2010-01-14 454.msg_control = NULL,
3a4d5c94e Michael S. Tsirkin 2010-01-14 455.msg_controllen = 0,
3a4d5c94e Michael S. Tsirkin 2010-01-14 456.msg_flags =
MSG_DONTWAIT,
3a4d5c94e Michael S. Tsirkin 2010-01-14 457};
3a4d5c94e Michael S. Tsirkin 2010-01-14 458size_t len, total_len = 0;
70181d512 Jason Wang 2013-04-10 459int err;
3a4d5c94e Michael S. Tsirkin 2010-01-14 460size_t hdr_size;
28457ee69 Arnd Bergmann 2010-03-09 461struct socket *sock;
fe729a57c Asias He 2013-05-06 462struct vhost_net_ubuf_ref
*uninitialized_var(ubufs);
cedb9bdce Michael S. Tsirkin 2012-12-06 463bool zcopy, zcopy_used;
28457ee69 Arnd Bergmann 2010-03-09 464
2e26af79b Asias He 2013-05-07 465mutex_lock(&vq->mutex);
2e26af79b Asias He 2013-05-07 466sock = vq->private_data;
3a4d5c94e Michael S. Tsirkin 2010-01-14 467if (!sock)
2e26af79b Asias He 2013-05-07 468goto out;
3a4d5c94e Michael S. Tsirkin 2010-01-14 469
6b1e6cc78 Jason Wang 2016-06-23 470if (!vq_iotlb_prefetch(vq))
6b1e6cc78 Jason Wang 2016-06-23 471goto out;
6b1e6cc78 Jason Wang 2016-06-23 472
8ea8cf89e Michael S. Tsirkin 2011-05-20 473vhost_disable_notify(&net->dev,
vq);
3a4d5c94e Michael S. Tsirkin 2010-01-14 474
81f95a558 Michael S. Tsirkin 2013-04-28 475hdr_size = nvq->vhost_hlen;
2839400f8 Asias He 2013-04-27 476zcopy = nvq->ubufs;
3a4d5c94e Michael S. Tsirkin 2010-01-14 477
3a4d5c94e Michael S. Tsirkin 2010-01-14 478for (;;) {
bab632d69 Michael S. Tsirkin 2011-07-18 479/* Release DMAs done
buffers first */
bab632d69 Michael S. Tsirkin 2011-07-18 480if (zcopy)
eaae8132e Michael S. Tsirkin 2012-11-01 481
vhost_ze
Re: [PATCH net-next 2/2] net: convert (struct ubuf_info)->refcnt to refcount_t
On Thu, 2017-08-31 at 18:45 -0400, Willem de Bruijn wrote:
> On Thu, Aug 31, 2017 at 4:30 PM, Eric Dumazet wrote:
> > refcount_t type and corresponding API should be
> > used instead of atomic_t when the variable is used as
> > a reference counter. This allows to avoid accidental
> > refcounter overflows that might lead to use-after-free
> > situations.
> >
> > Signed-off-by: Eric Dumazet
> > ---
> > include/linux/skbuff.h | 5 +++--
> > net/core/skbuff.c | 8
> > 2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index
> > 7594e19bce622a38dc39c054093c3da15b99b67b..316a92b45351f53709886ee0099cbc83b66f1b15
> > 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -22,6 +22,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -456,7 +457,7 @@ struct ubuf_info {
> > u32 bytelen;
> > };
> > };
> > - atomic_t refcnt;
> > + refcount_t refcnt;
> >
> > struct mmpin {
> > struct user_struct *user;
> > @@ -472,7 +473,7 @@ struct ubuf_info *sock_zerocopy_realloc(struct sock
> > *sk, size_t size,
> >
> > static inline void sock_zerocopy_get(struct ubuf_info *uarg)
> > {
> > - atomic_inc(&uarg->refcnt);
> > + refcount_inc(&uarg->refcnt);
> > }
> >
> > void sock_zerocopy_put(struct ubuf_info *uarg);
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index
> > 65b9ca3945f8fd2b1bef4aef5dd774be04e5d128..ed86ca9afd9d8d1ac47983acf6006c179285a612
> > 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -963,7 +963,7 @@ struct ubuf_info *sock_zerocopy_alloc(struct sock *sk,
> > size_t size)
> > uarg->len = 1;
> > uarg->bytelen = size;
> > uarg->zerocopy = 1;
> > - atomic_set(&uarg->refcnt, 1);
> > + refcount_set(&uarg->refcnt, 1);
> > sock_hold(sk);
> >
> > return uarg;
> > @@ -1086,7 +1086,7 @@ EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
> >
> > void sock_zerocopy_put(struct ubuf_info *uarg)
> > {
> > - if (uarg && atomic_dec_and_test(&uarg->refcnt)) {
> > + if (uarg && refcount_dec_and_test(&uarg->refcnt)) {
> > if (uarg->callback)
> > uarg->callback(uarg, uarg->zerocopy);
> > else
> > @@ -1108,7 +1108,7 @@ void sock_zerocopy_put_abort(struct ubuf_info *uarg)
> > * avoid an skb send inside the main loop triggering uarg
> > free.
> > */
> > if (sk->sk_type != SOCK_STREAM)
> > - atomic_inc(&uarg->refcnt);
> > + refcount_inc(&uarg->refcnt);
>
> This would be a 0 -> 1 transition. It is taken only on the error path from
> a socket that does not take an explicit hold at the start of sendmsg.
>
Arg.
> The code is vestigial, really, as the final patchset only includes tcp.
> It is safe to leave as is for now, or I can remove it.
>
OK I probably should remove this chunk in patch 1/2 then.
> In 1f8b977ab32d ("sock: enable MSG_ZEROCOPY") I also updated
> other users of ubuf_info to initialize refcnt. We probably also want to
> convert the call in handle_tx in drivers/vhost/net.c.
Thanks ! :)
Re: [PATCH net-next 2/2] net: convert (struct ubuf_info)->refcnt to refcount_t
On Thu, Aug 31, 2017 at 4:30 PM, Eric Dumazet wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
>
> Signed-off-by: Eric Dumazet
> ---
> include/linux/skbuff.h | 5 +++--
> net/core/skbuff.c | 8
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index
> 7594e19bce622a38dc39c054093c3da15b99b67b..316a92b45351f53709886ee0099cbc83b66f1b15
> 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -456,7 +457,7 @@ struct ubuf_info {
> u32 bytelen;
> };
> };
> - atomic_t refcnt;
> + refcount_t refcnt;
>
> struct mmpin {
> struct user_struct *user;
> @@ -472,7 +473,7 @@ struct ubuf_info *sock_zerocopy_realloc(struct sock *sk,
> size_t size,
>
> static inline void sock_zerocopy_get(struct ubuf_info *uarg)
> {
> - atomic_inc(&uarg->refcnt);
> + refcount_inc(&uarg->refcnt);
> }
>
> void sock_zerocopy_put(struct ubuf_info *uarg);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index
> 65b9ca3945f8fd2b1bef4aef5dd774be04e5d128..ed86ca9afd9d8d1ac47983acf6006c179285a612
> 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -963,7 +963,7 @@ struct ubuf_info *sock_zerocopy_alloc(struct sock *sk,
> size_t size)
> uarg->len = 1;
> uarg->bytelen = size;
> uarg->zerocopy = 1;
> - atomic_set(&uarg->refcnt, 1);
> + refcount_set(&uarg->refcnt, 1);
> sock_hold(sk);
>
> return uarg;
> @@ -1086,7 +1086,7 @@ EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
>
> void sock_zerocopy_put(struct ubuf_info *uarg)
> {
> - if (uarg && atomic_dec_and_test(&uarg->refcnt)) {
> + if (uarg && refcount_dec_and_test(&uarg->refcnt)) {
> if (uarg->callback)
> uarg->callback(uarg, uarg->zerocopy);
> else
> @@ -1108,7 +1108,7 @@ void sock_zerocopy_put_abort(struct ubuf_info *uarg)
> * avoid an skb send inside the main loop triggering uarg
> free.
> */
> if (sk->sk_type != SOCK_STREAM)
> - atomic_inc(&uarg->refcnt);
> + refcount_inc(&uarg->refcnt);
This would be a 0 -> 1 transition. It is taken only on the error path from
a socket that does not take an explicit hold at the start of sendmsg.
The code is vestigial, really, as the final patchset only includes tcp.
It is safe to leave as is for now, or I can remove it.
In 1f8b977ab32d ("sock: enable MSG_ZEROCOPY") I also updated
other users of ubuf_info to initialize refcnt. We probably also want to
convert the call in handle_tx in drivers/vhost/net.c.
[PATCH net-next 2/2] net: convert (struct ubuf_info)->refcnt to refcount_t
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Eric Dumazet
---
include/linux/skbuff.h | 5 +++--
net/core/skbuff.c | 8
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index
7594e19bce622a38dc39c054093c3da15b99b67b..316a92b45351f53709886ee0099cbc83b66f1b15
100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
@@ -456,7 +457,7 @@ struct ubuf_info {
u32 bytelen;
};
};
- atomic_t refcnt;
+ refcount_t refcnt;
struct mmpin {
struct user_struct *user;
@@ -472,7 +473,7 @@ struct ubuf_info *sock_zerocopy_realloc(struct sock *sk,
size_t size,
static inline void sock_zerocopy_get(struct ubuf_info *uarg)
{
- atomic_inc(&uarg->refcnt);
+ refcount_inc(&uarg->refcnt);
}
void sock_zerocopy_put(struct ubuf_info *uarg);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index
65b9ca3945f8fd2b1bef4aef5dd774be04e5d128..ed86ca9afd9d8d1ac47983acf6006c179285a612
100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -963,7 +963,7 @@ struct ubuf_info *sock_zerocopy_alloc(struct sock *sk,
size_t size)
uarg->len = 1;
uarg->bytelen = size;
uarg->zerocopy = 1;
- atomic_set(&uarg->refcnt, 1);
+ refcount_set(&uarg->refcnt, 1);
sock_hold(sk);
return uarg;
@@ -1086,7 +1086,7 @@ EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
void sock_zerocopy_put(struct ubuf_info *uarg)
{
- if (uarg && atomic_dec_and_test(&uarg->refcnt)) {
+ if (uarg && refcount_dec_and_test(&uarg->refcnt)) {
if (uarg->callback)
uarg->callback(uarg, uarg->zerocopy);
else
@@ -1108,7 +1108,7 @@ void sock_zerocopy_put_abort(struct ubuf_info *uarg)
* avoid an skb send inside the main loop triggering uarg free.
*/
if (sk->sk_type != SOCK_STREAM)
- atomic_inc(&uarg->refcnt);
+ refcount_inc(&uarg->refcnt);
sock_zerocopy_put(uarg);
}
@@ -1490,7 +1490,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int
ntail,
if (skb_orphan_frags(skb, gfp_mask))
goto nofrags;
if (skb_zcopy(skb))
- atomic_inc(&skb_uarg(skb)->refcnt);
+ refcount_inc(&skb_uarg(skb)->refcnt);
for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
skb_frag_ref(skb, i);
--
2.14.1.581.gf28d330327-goog
