netdev - Re: [PATCH net-next v2 1/9] xen-netback: Introduce TX grant map definitions

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]

Message-ID: <52AF1A84.3090304@citrix.com>
Date:	Mon, 16 Dec 2013 15:21:40 +0000
From:	Zoltan Kiss <zoltan.kiss@...rix.com>
To:	Wei Liu <wei.liu2@...rix.com>
CC:	<ian.campbell@...rix.com>, <xen-devel@...ts.xenproject.org>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<jonathan.davies@...rix.com>
Subject: Re: [PATCH net-next v2 1/9] xen-netback: Introduce TX grant map definitions

On 13/12/13 19:14, Wei Liu wrote:
>>>> +	spin_lock_irqsave(&vif->dealloc_lock, flags);
>>>> > >>+	do {
>>>> > >>+		pending_idx = ubuf->desc;
>>>> > >>+		ubuf = (struct ubuf_info *) ubuf->ctx;
>>>> > >>+		index = pending_index(vif->dealloc_prod);
>>>> > >>+		vif->dealloc_ring[index] = pending_idx;
>>>> > >>+		/* Sync with xenvif_tx_action_dealloc:
>>>> > >>+		 * insert idx then incr producer.
>>>> > >>+		 */
>>>> > >>+		smp_wmb();
>>>> > >>+		vif->dealloc_prod++;
>>>> > >>+	} while (ubuf);
>>>> > >>+	wake_up(&vif->dealloc_wq);
>>>> > >>+	spin_unlock_irqrestore(&vif->dealloc_lock, flags);
> [...]
>>> > >
>>>> > >>+		smp_rmb();
>>>> > >>+
>>>> > >>+		while (dc != dp) {
>>>> > >>+			pending_idx =
>>>> > >>+				vif->dealloc_ring[pending_index(dc++)];
>>>> > >>+
>>>> > >>+			/* Already unmapped? */
>>>> > >>+			if (vif->grant_tx_handle[pending_idx] ==
>>>> > >>+				NETBACK_INVALID_HANDLE) {
>>>> > >>+				netdev_err(vif->dev,
>>>> > >>+					"Trying to unmap invalid handle! "
>>>> > >>+					"pending_idx: %x\n", pending_idx);
>>>> > >>+				continue;
>>>> > >>+			}
>>> > >
>>> > >Should this be BUG_ON? AIUI this kthread should be the only one doing
>>> > >unmap, right?
>> >The NAPI instance can do it as well if it is a small packet fits
>> >into PKT_PROT_LEN. But still this scenario shouldn't really happen,
>> >I was just not sure we have to crash immediately. Maybe handle it as
>> >a fatal error and destroy the vif?
>> >
> It depends. If this is within the trust boundary, i.e. everything at the
> stage should have been sanitized then we should BUG_ON because there's
> clearly a bug somewhere in the sanitization process, or in the
> interaction of various backend routines.

My understanding is that crashing should be avoided if we can bail out 
somehow. At this point there is clearly a bug in netback somewhere, 
something unmapped that page before it should have happened, or at least 
that array get corrupted somehow. However there is a chance that 
xenvif_fatal_tx_err() can contain the issue, and the rest of the system 
can go unaffected.

Zoli
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html