linux-kernel - RE: [PATCH v2] Drivers: hv: vmbus: Expose counters for interrupts and full conditions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Jan 2019 03:58:34 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     Michael Kelley <mikelley@...rosoft.com>,
        kimbrownkd <kimbrownkd@...il.com>,
        Long Li <longli@...rosoft.com>,
        Sasha Levin <Alexander.Levin@...rosoft.com>
CC:     KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] Drivers: hv: vmbus: Expose counters for interrupts and
 full conditions

> From: Michael Kelley <mikelley@...rosoft.com>
> Sent: Saturday, January 5, 2019 1:01 PM
> > From: Kimberly Brown <kimbrownkd@...il.com>  Sent: Friday, January 4,
>  > 2019 8:35 PM
> >
> >  static inline void set_channel_pending_send_size(struct vmbus_channel *c,
> >  						 u32 size)
> >  {
> > +	if (size) {
> > +		++c->out_full_total;
> > +
> > +		if (!c->out_full_flag) {
> > +			++c->out_full_first;
> > +			c->out_full_flag = true;
> > +		}
> > +	} else {
> > +		c->out_full_flag = false;
> > +	}
> > +
> >  	c->outbound.ring_buffer->pending_send_sz = size;
> >  }
> >
> 
> I think there may be an atomicity problem with the above code.  I looked
> in the hv_sock code, and didn't see any locks being held when
> set_channel_pending_send_size() is called.   The original code doesn't need
> a lock because it is just storing a single value into pending_send_sz.
>
> In the similar code in hv_ringbuffer_write(), the ring buffer spin lock is held
> while the counts are incremented and the out_full_flag is maintained, so all is
> good there.  But some locking may be needed here.  Dexuan knows the
> hv_sock
> code best and can comment on whether there is any higher level
> synchronization
> that prevents multiple threads from running the above code on the same
> channel.
> Even if there is such higher level synchronization, this code probably shouldn't
> depend on it for correctness.
> 
> Michael

Yes, there is indeed a higher level per-"sock" lock, e.g. in the code path
vsock_stream_sendmsg() -> vsock_stream_has_space() -> 
transport->stream_has_space() -> hvs_stream_has_space() -> 
hvs_set_channel_pending_send_size(), we acquire the lock by
lock_sock(sk) at the beginning of vsock_stream_sendmsg(), and call
release_sock(sk) at the end of the function.

So we don't have an atomicity issue here for hv_sock, which is the only user
of set_channel_pending_send_size(), so far.

Thanks,
-- Dexuan