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:   Sat, 5 Jan 2019 21:01:07 +0000
From:   Michael Kelley <mikelley@...rosoft.com>
To:     kimbrownkd <kimbrownkd@...il.com>, Long Li <longli@...rosoft.com>,
        Sasha Levin <Alexander.Levin@...rosoft.com>,
        Dexuan Cui <decui@...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: 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