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

[Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel