[PATCH] Don't allow sharing of tx skbs on xen-netfront

[PATCH] Don't allow sharing of tx skbs on xen-netfront

[Neil Horman]

It was pointed out to me recently that the xen-netfront driver can't safely
support shared skbs on transmit, since, while it doesn't maintain skb state
directly, it does pass a pointer to the skb to the hypervisor via a list, and
the hypervisor may expect the contents of the skb to remain stable.  Clearing
the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: xen-devel@lists.xensource.com
---
 drivers/net/xen-netfront.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 226faab..fb1077b 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1252,6 +1252,12 @@ static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev
 		return ERR_PTR(-ENOMEM);
 	}
 
+	/*
+	 * Since frames remain on a queue after a return from xennet_start_xmit,
+	 * we can't support tx shared skbs
+	 */
+	netdev->priv_flags &= ~IFF_TX_SKB_SHARING;
+
 	np                   = netdev_priv(netdev);
 	np->xbdev            = dev;
 
-- 
1.7.6.4

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon, 14 Nov 2011 14:22:24 -0500

> It was pointed out to me recently that the xen-netfront driver can't safely
> support shared skbs on transmit, since, while it doesn't maintain skb state
> directly, it does pass a pointer to the skb to the hypervisor via a list, and
> the hypervisor may expect the contents of the skb to remain stable.  Clearing
> the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Please put an appropriate prefix into the subject lines of your patch
submissions.  In this case "[PATCH] xen-netfront: ..." would be appropriate.

I've been letting you get away with this for the past few weeks and I've
decided that it's your turn to start getting this right :-)

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[Neil Horman]

On Mon, Nov 14, 2011 at 02:27:16PM -0500, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Mon, 14 Nov 2011 14:22:24 -0500
> 
> > It was pointed out to me recently that the xen-netfront driver can't safely
> > support shared skbs on transmit, since, while it doesn't maintain skb state
> > directly, it does pass a pointer to the skb to the hypervisor via a list, and
> > the hypervisor may expect the contents of the skb to remain stable.  Clearing
> > the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Please put an appropriate prefix into the subject lines of your patch
> submissions.  In this case "[PATCH] xen-netfront: ..." would be appropriate.
> 
> I've been letting you get away with this for the past few weeks and I've
> decided that it's your turn to start getting this right :-)
Jeez Dave, I got it right on the cgroups post, you want consistency now
too?  :).

Apologies, I need to consult a checklist for myself prior to sending stuff.

Best
Neil



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

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[Ian Campbell]

On Mon, 2011-11-14 at 14:22 -0500, Neil Horman wrote:
> It was pointed out to me recently that the xen-netfront driver can't safely
> support shared skbs on transmit, since, while it doesn't maintain skb state
> directly, it does pass a pointer to the skb to the hypervisor via a list, and
> the hypervisor may expect the contents of the skb to remain stable.  Clearing
> the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.

What are the actual constraints here? The skb is used as a handle to the
skb->data and shinfo (frags) and to complete at the end. It's actually
those which are passed to the hypervisor (effectively the same as
passing those addresses to the h/w for DMA).

Which parts of the skb are expected/allowed to not remain stable?

(Appologies if the above seems naive, I seem to have missed the
introduction of shared tx skbs and IFF_TX_SKB_SHARING)

Ian.

> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: xen-devel@lists.xensource.com
> ---
>  drivers/net/xen-netfront.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 226faab..fb1077b 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -1252,6 +1252,12 @@ static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> +	/*
> +	 * Since frames remain on a queue after a return from xennet_start_xmit,
> +	 * we can't support tx shared skbs
> +	 */
> +	netdev->priv_flags &= ~IFF_TX_SKB_SHARING;
> +
>  	np                   = netdev_priv(netdev);
>  	np->xbdev            = dev;
>  

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[Neil Horman]

On Thu, Nov 17, 2011 at 03:20:38PM +0000, Ian Campbell wrote:
> On Mon, 2011-11-14 at 14:22 -0500, Neil Horman wrote:
> > It was pointed out to me recently that the xen-netfront driver can't safely
> > support shared skbs on transmit, since, while it doesn't maintain skb state
> > directly, it does pass a pointer to the skb to the hypervisor via a list, and
> > the hypervisor may expect the contents of the skb to remain stable.  Clearing
> > the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.
> 
> What are the actual constraints here? The skb is used as a handle to the
> skb->data and shinfo (frags) and to complete at the end. It's actually
> those which are passed to the hypervisor (effectively the same as
> passing those addresses to the h/w for DMA).
> 
> Which parts of the skb are expected/allowed to not remain stable?
> 
> (Appologies if the above seems naive, I seem to have missed the
> introduction of shared tx skbs and IFF_TX_SKB_SHARING)
> 
Its ok, this is the most accurate description from the previous threads on the
subject:
http://lists.openwall.net/netdev/2011/08/22/63

The basic problem boils down the notion that some drivers, when they receive an
skb in their xmit paths, presume to have sole ownership of the skb, and as a
result may do things like add the skb to a list, or otherwise store stateful
data in the skb.  If the skb is shared, thats unsafe to do, as the stack still
holds a reference to the skb, and make make changes without serializing them
against the driver.  So we have to flag those drivers which preform these kinds
of actions.  xen-netfront doesn't strictly speaking modify any state directly ni
an skb, but it does place a pointer to the skb in a data structure here:

np->tx_skbs[id].skb = skb;

Which then gets handed off to the hypervisior.  Since the hypervisor now has
access to that skb pointer, and we can't be sure (from the guest perspective),
what it does with that information, it would be better to be safe by disallowing
shared skbs in this path.

Neil

> Ian.
> 
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > CC: xen-devel@lists.xensource.com
> > ---
> >  drivers/net/xen-netfront.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> > index 226faab..fb1077b 100644
> > --- a/drivers/net/xen-netfront.c
> > +++ b/drivers/net/xen-netfront.c
> > @@ -1252,6 +1252,12 @@ static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev
> >  		return ERR_PTR(-ENOMEM);
> >  	}
> >  
> > +	/*
> > +	 * Since frames remain on a queue after a return from xennet_start_xmit,
> > +	 * we can't support tx shared skbs
> > +	 */
> > +	netdev->priv_flags &= ~IFF_TX_SKB_SHARING;
> > +
> >  	np                   = netdev_priv(netdev);
> >  	np->xbdev            = dev;
> >  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[Ian Campbell]

On Thu, 2011-11-17 at 19:25 +0000, Neil Horman wrote:
> On Thu, Nov 17, 2011 at 03:20:38PM +0000, Ian Campbell wrote:
> > On Mon, 2011-11-14 at 14:22 -0500, Neil Horman wrote:
> > > It was pointed out to me recently that the xen-netfront driver can't safely
> > > support shared skbs on transmit, since, while it doesn't maintain skb state
> > > directly, it does pass a pointer to the skb to the hypervisor via a list, and
> > > the hypervisor may expect the contents of the skb to remain stable.  Clearing
> > > the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.
> > 
> > What are the actual constraints here? The skb is used as a handle to the
> > skb->data and shinfo (frags) and to complete at the end. It's actually
> > those which are passed to the hypervisor (effectively the same as
> > passing those addresses to the h/w for DMA).
> > 
> > Which parts of the skb are expected/allowed to not remain stable?
> > 
> > (Appologies if the above seems naive, I seem to have missed the
> > introduction of shared tx skbs and IFF_TX_SKB_SHARING)
> > 
> Its ok, this is the most accurate description from the previous threads on the
> subject:
> http://lists.openwall.net/netdev/2011/08/22/63
> 
> The basic problem boils down the notion that some drivers, when they receive an
> skb in their xmit paths, presume to have sole ownership of the skb, and as a
> result may do things like add the skb to a list, or otherwise store stateful
> data in the skb.  If the skb is shared, thats unsafe to do, as the stack still
> holds a reference to the skb, and make make changes without serializing them
> against the driver.  So we have to flag those drivers which preform these kinds
> of actions.  xen-netfront doesn't strictly speaking modify any state directly ni
> an skb, but it does place a pointer to the skb in a data structure here:
> 
> np->tx_skbs[id].skb = skb;
> 
> Which then gets handed off to the hypervisior.  Since the hypervisor now has
> access to that skb pointer, and we can't be sure (from the guest perspective),
> what it does with that information, it would be better to be safe by disallowing
> shared skbs in this path.

The skb pointer itself doesn't get given to the backend/hypervisor. The
page which skb->data refers to is granted to the backend domain, as are
the pages in the frags.

I think we only need to be sure that the frontend doesn't rely on
anything in the skb itself, right? Does skb->data or shinfo count from
that perspective?

Ian.

> 
> Neil
> 
> > Ian.
> > 
> > > 
> > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > CC: "David S. Miller" <davem@davemloft.net>
> > > CC: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> > > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > CC: xen-devel@lists.xensource.com
> > > ---
> > >  drivers/net/xen-netfront.c |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> > > index 226faab..fb1077b 100644
> > > --- a/drivers/net/xen-netfront.c
> > > +++ b/drivers/net/xen-netfront.c
> > > @@ -1252,6 +1252,12 @@ static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev
> > >  		return ERR_PTR(-ENOMEM);
> > >  	}
> > >  
> > > +	/*
> > > +	 * Since frames remain on a queue after a return from xennet_start_xmit,
> > > +	 * we can't support tx shared skbs
> > > +	 */
> > > +	netdev->priv_flags &= ~IFF_TX_SKB_SHARING;
> > > +
> > >  	np                   = netdev_priv(netdev);
> > >  	np->xbdev            = dev;
> > >  
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[Neil Horman]

On Thu, Nov 17, 2011 at 08:17:01PM +0000, Ian Campbell wrote:
> On Thu, 2011-11-17 at 19:25 +0000, Neil Horman wrote:
> > On Thu, Nov 17, 2011 at 03:20:38PM +0000, Ian Campbell wrote:
> > > On Mon, 2011-11-14 at 14:22 -0500, Neil Horman wrote:
> > > > It was pointed out to me recently that the xen-netfront driver can't safely
> > > > support shared skbs on transmit, since, while it doesn't maintain skb state
> > > > directly, it does pass a pointer to the skb to the hypervisor via a list, and
> > > > the hypervisor may expect the contents of the skb to remain stable.  Clearing
> > > > the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.
> > > 
> > > What are the actual constraints here? The skb is used as a handle to the
> > > skb->data and shinfo (frags) and to complete at the end. It's actually
> > > those which are passed to the hypervisor (effectively the same as
> > > passing those addresses to the h/w for DMA).
> > > 
> > > Which parts of the skb are expected/allowed to not remain stable?
> > > 
> > > (Appologies if the above seems naive, I seem to have missed the
> > > introduction of shared tx skbs and IFF_TX_SKB_SHARING)
> > > 
> > Its ok, this is the most accurate description from the previous threads on the
> > subject:
> > http://lists.openwall.net/netdev/2011/08/22/63
> > 
> > The basic problem boils down the notion that some drivers, when they receive an
> > skb in their xmit paths, presume to have sole ownership of the skb, and as a
> > result may do things like add the skb to a list, or otherwise store stateful
> > data in the skb.  If the skb is shared, thats unsafe to do, as the stack still
> > holds a reference to the skb, and make make changes without serializing them
> > against the driver.  So we have to flag those drivers which preform these kinds
> > of actions.  xen-netfront doesn't strictly speaking modify any state directly ni
> > an skb, but it does place a pointer to the skb in a data structure here:
> > 
> > np->tx_skbs[id].skb = skb;
> > 
> > Which then gets handed off to the hypervisior.  Since the hypervisor now has
> > access to that skb pointer, and we can't be sure (from the guest perspective),
> > what it does with that information, it would be better to be safe by disallowing
> > shared skbs in this path.
> 
> The skb pointer itself doesn't get given to the backend/hypervisor. The
> page which skb->data refers to is granted to the backend domain, as are
> the pages in the frags.
> 
> I think we only need to be sure that the frontend doesn't rely on
> anything in the skb itself, right? Does skb->data or shinfo count from
> that perspective?
shinfo is definately a problem, as other devices may make modifications to it.
skb->data is probably safer, but is also potentially suspect (for instance if
another device appends an additional header to the data for instance)
Neil

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[Ian Campbell]

On Thu, 2011-11-17 at 20:45 +0000, Neil Horman wrote:
> On Thu, Nov 17, 2011 at 08:17:01PM +0000, Ian Campbell wrote:
> > On Thu, 2011-11-17 at 19:25 +0000, Neil Horman wrote:
> > > On Thu, Nov 17, 2011 at 03:20:38PM +0000, Ian Campbell wrote:
> > > > On Mon, 2011-11-14 at 14:22 -0500, Neil Horman wrote:
> > > > > It was pointed out to me recently that the xen-netfront driver can't safely
> > > > > support shared skbs on transmit, since, while it doesn't maintain skb state
> > > > > directly, it does pass a pointer to the skb to the hypervisor via a list, and
> > > > > the hypervisor may expect the contents of the skb to remain stable.  Clearing
> > > > > the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.
> > > > 
> > > > What are the actual constraints here? The skb is used as a handle to the
> > > > skb->data and shinfo (frags) and to complete at the end. It's actually
> > > > those which are passed to the hypervisor (effectively the same as
> > > > passing those addresses to the h/w for DMA).
> > > > 
> > > > Which parts of the skb are expected/allowed to not remain stable?
> > > > 
> > > > (Appologies if the above seems naive, I seem to have missed the
> > > > introduction of shared tx skbs and IFF_TX_SKB_SHARING)
> > > > 
> > > Its ok, this is the most accurate description from the previous threads on the
> > > subject:
> > > 2
> > > 
> > > The basic problem boils down the notion that some drivers, when they receive an
> > > skb in their xmit paths, presume to have sole ownership of the skb, and as a
> > > result may do things like add the skb to a list, or otherwise store stateful
> > > data in the skb.  If the skb is shared, thats unsafe to do, as the stack still
> > > holds a reference to the skb, and make make changes without serializing them
> > > against the driver.  So we have to flag those drivers which preform these kinds
> > > of actions.  xen-netfront doesn't strictly speaking modify any state directly ni
> > > an skb, but it does place a pointer to the skb in a data structure here:
> > > 
> > > np->tx_skbs[id].skb = skb;
> > > 
> > > Which then gets handed off to the hypervisior.  Since the hypervisor now has
> > > access to that skb pointer, and we can't be sure (from the guest perspective),
> > > what it does with that information, it would be better to be safe by disallowing
> > > shared skbs in this path.
> > 
> > The skb pointer itself doesn't get given to the backend/hypervisor. The
> > page which skb->data refers to is granted to the backend domain, as are
> > the pages in the frags.
> > 
> > I think we only need to be sure that the frontend doesn't rely on
> > anything in the skb itself, right? Does skb->data or shinfo count from
> > that perspective?
> shinfo is definately a problem, as other devices may make modifications to it.
> skb->data is probably safer, but is also potentially suspect (for instance if
> another device appends an additional header to the data for instance)

A device is allowed to rely on these things being stable while in its
start_xmit, right? (otherwise I don't see how any device can ever
cope...).

netfront only uses shinfo and ->data during start_xmit in order to
create the necessary grant reference (which can be thought of as a DMA
address passed to the virtual hardware). The only use of the stashed skb
pointer outside of this are to dev_kfree_skb on tx completion (from
either tx_buf_gc (normal completion) or release_tx_buf ("hardware"
reset).

Ian.

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[Neil Horman]

On Fri, Nov 18, 2011 at 10:30:13AM +0000, Ian Campbell wrote:
> On Thu, 2011-11-17 at 20:45 +0000, Neil Horman wrote:
> > On Thu, Nov 17, 2011 at 08:17:01PM +0000, Ian Campbell wrote:
> > > On Thu, 2011-11-17 at 19:25 +0000, Neil Horman wrote:
> > > > On Thu, Nov 17, 2011 at 03:20:38PM +0000, Ian Campbell wrote:
> > > > > On Mon, 2011-11-14 at 14:22 -0500, Neil Horman wrote:
> > > > > > It was pointed out to me recently that the xen-netfront driver can't safely
> > > > > > support shared skbs on transmit, since, while it doesn't maintain skb state
> > > > > > directly, it does pass a pointer to the skb to the hypervisor via a list, and
> > > > > > the hypervisor may expect the contents of the skb to remain stable.  Clearing
> > > > > > the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.
> > > > > 
> > > > > What are the actual constraints here? The skb is used as a handle to the
> > > > > skb->data and shinfo (frags) and to complete at the end. It's actually
> > > > > those which are passed to the hypervisor (effectively the same as
> > > > > passing those addresses to the h/w for DMA).
> > > > > 
> > > > > Which parts of the skb are expected/allowed to not remain stable?
> > > > > 
> > > > > (Appologies if the above seems naive, I seem to have missed the
> > > > > introduction of shared tx skbs and IFF_TX_SKB_SHARING)
> > > > > 
> > > > Its ok, this is the most accurate description from the previous threads on the
> > > > subject:
> > > > 2
> > > > 
> > > > The basic problem boils down the notion that some drivers, when they receive an
> > > > skb in their xmit paths, presume to have sole ownership of the skb, and as a
> > > > result may do things like add the skb to a list, or otherwise store stateful
> > > > data in the skb.  If the skb is shared, thats unsafe to do, as the stack still
> > > > holds a reference to the skb, and make make changes without serializing them
> > > > against the driver.  So we have to flag those drivers which preform these kinds
> > > > of actions.  xen-netfront doesn't strictly speaking modify any state directly ni
> > > > an skb, but it does place a pointer to the skb in a data structure here:
> > > > 
> > > > np->tx_skbs[id].skb = skb;
> > > > 
> > > > Which then gets handed off to the hypervisior.  Since the hypervisor now has
> > > > access to that skb pointer, and we can't be sure (from the guest perspective),
> > > > what it does with that information, it would be better to be safe by disallowing
> > > > shared skbs in this path.
> > > 
> > > The skb pointer itself doesn't get given to the backend/hypervisor. The
> > > page which skb->data refers to is granted to the backend domain, as are
> > > the pages in the frags.
> > > 
> > > I think we only need to be sure that the frontend doesn't rely on
> > > anything in the skb itself, right? Does skb->data or shinfo count from
> > > that perspective?
> > shinfo is definately a problem, as other devices may make modifications to it.
> > skb->data is probably safer, but is also potentially suspect (for instance if
> > another device appends an additional header to the data for instance)
> 
> A device is allowed to rely on these things being stable while in its
> start_xmit, right? (otherwise I don't see how any device can ever
> cope...).
> 
While the start_xmit routine is executing, yes.  Its only after the driver
returns, that it can have no expectation of an skb's data to remain stable.

> netfront only uses shinfo and ->data during start_xmit in order to
> create the necessary grant reference (which can be thought of as a DMA
> address passed to the virtual hardware). The only use of the stashed skb
> pointer outside of this are to dev_kfree_skb on tx completion (from
> either tx_buf_gc (normal completion) or release_tx_buf ("hardware"
> reset).
> 
Ok, if you're certain you can guarantee that the hypervisior makes no inspection
of the skb after the return from the driver, then you're safe
Neil

> Ian.
> 
> 

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[Ian Campbell]

On Fri, 2011-11-18 at 11:48 +0000, Neil Horman wrote:
> On Fri, Nov 18, 2011 at 10:30:13AM +0000, Ian Campbell wrote:
> > On Thu, 2011-11-17 at 20:45 +0000, Neil Horman wrote:
> > > On Thu, Nov 17, 2011 at 08:17:01PM +0000, Ian Campbell wrote:
> > > > On Thu, 2011-11-17 at 19:25 +0000, Neil Horman wrote:
> > > > > On Thu, Nov 17, 2011 at 03:20:38PM +0000, Ian Campbell wrote:
> > > > > > On Mon, 2011-11-14 at 14:22 -0500, Neil Horman wrote:
> > > > > > > It was pointed out to me recently that the xen-netfront driver can't safely
> > > > > > > support shared skbs on transmit, since, while it doesn't maintain skb state
> > > > > > > directly, it does pass a pointer to the skb to the hypervisor via a list, and
> > > > > > > the hypervisor may expect the contents of the skb to remain stable.  Clearing
> > > > > > > the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.
> > > > > > 
> > > > > > What are the actual constraints here? The skb is used as a handle to the
> > > > > > skb->data and shinfo (frags) and to complete at the end. It's actually
> > > > > > those which are passed to the hypervisor (effectively the same as
> > > > > > passing those addresses to the h/w for DMA).
> > > > > > 
> > > > > > Which parts of the skb are expected/allowed to not remain stable?
> > > > > > 
> > > > > > (Appologies if the above seems naive, I seem to have missed the
> > > > > > introduction of shared tx skbs and IFF_TX_SKB_SHARING)
> > > > > > 
> > > > > Its ok, this is the most accurate description from the previous threads on the
> > > > > subject:
> > > > > 2
> > > > > 
> > > > > The basic problem boils down the notion that some drivers, when they receive an
> > > > > skb in their xmit paths, presume to have sole ownership of the skb, and as a
> > > > > result may do things like add the skb to a list, or otherwise store stateful
> > > > > data in the skb.  If the skb is shared, thats unsafe to do, as the stack still
> > > > > holds a reference to the skb, and make make changes without serializing them
> > > > > against the driver.  So we have to flag those drivers which preform these kinds
> > > > > of actions.  xen-netfront doesn't strictly speaking modify any state directly ni
> > > > > an skb, but it does place a pointer to the skb in a data structure here:
> > > > > 
> > > > > np->tx_skbs[id].skb = skb;
> > > > > 
> > > > > Which then gets handed off to the hypervisior.  Since the hypervisor now has
> > > > > access to that skb pointer, and we can't be sure (from the guest perspective),
> > > > > what it does with that information, it would be better to be safe by disallowing
> > > > > shared skbs in this path.
> > > > 
> > > > The skb pointer itself doesn't get given to the backend/hypervisor. The
> > > > page which skb->data refers to is granted to the backend domain, as are
> > > > the pages in the frags.
> > > > 
> > > > I think we only need to be sure that the frontend doesn't rely on
> > > > anything in the skb itself, right? Does skb->data or shinfo count from
> > > > that perspective?
> > > shinfo is definately a problem, as other devices may make modifications to it.
> > > skb->data is probably safer, but is also potentially suspect (for instance if
> > > another device appends an additional header to the data for instance)
> > 
> > A device is allowed to rely on these things being stable while in its
> > start_xmit, right? (otherwise I don't see how any device can ever
> > cope...).
> > 
> While the start_xmit routine is executing, yes.  Its only after the driver
> returns, that it can have no expectation of an skb's data to remain stable.
> 
> > netfront only uses shinfo and ->data during start_xmit in order to
> > create the necessary grant reference (which can be thought of as a DMA
> > address passed to the virtual hardware). The only use of the stashed skb
> > pointer outside of this are to dev_kfree_skb on tx completion (from
> > either tx_buf_gc (normal completion) or release_tx_buf ("hardware"
> > reset).
> > 
> Ok, if you're certain you can guarantee that the hypervisior makes no inspection
> of the skb after the return from the driver, then you're safe

I believe this is the case, all that is exposed to the backend is the
pfn, offset and length of the skb->data and frags at the time start_xmit
was called.

> Neil
> 
> > Ian.
> > 
> > 

Re: [PATCH] Don't allow sharing of tx skbs on xen-netfront

[Neil Horman]

On Fri, Nov 18, 2011 at 11:53:09AM +0000, Ian Campbell wrote:
> On Fri, 2011-11-18 at 11:48 +0000, Neil Horman wrote:
> > On Fri, Nov 18, 2011 at 10:30:13AM +0000, Ian Campbell wrote:
> > > On Thu, 2011-11-17 at 20:45 +0000, Neil Horman wrote:
> > > > On Thu, Nov 17, 2011 at 08:17:01PM +0000, Ian Campbell wrote:
> > > > > On Thu, 2011-11-17 at 19:25 +0000, Neil Horman wrote:
> > > > > > On Thu, Nov 17, 2011 at 03:20:38PM +0000, Ian Campbell wrote:
> > > > > > > On Mon, 2011-11-14 at 14:22 -0500, Neil Horman wrote:
> > > > > > > > It was pointed out to me recently that the xen-netfront driver can't safely
> > > > > > > > support shared skbs on transmit, since, while it doesn't maintain skb state
> > > > > > > > directly, it does pass a pointer to the skb to the hypervisor via a list, and
> > > > > > > > the hypervisor may expect the contents of the skb to remain stable.  Clearing
> > > > > > > > the IFF_TX_SKB_SHARING flag after the call to alloc_etherdev to make it safe.
> > > > > > > 
> > > > > > > What are the actual constraints here? The skb is used as a handle to the
> > > > > > > skb->data and shinfo (frags) and to complete at the end. It's actually
> > > > > > > those which are passed to the hypervisor (effectively the same as
> > > > > > > passing those addresses to the h/w for DMA).
> > > > > > > 
> > > > > > > Which parts of the skb are expected/allowed to not remain stable?
> > > > > > > 
> > > > > > > (Appologies if the above seems naive, I seem to have missed the
> > > > > > > introduction of shared tx skbs and IFF_TX_SKB_SHARING)
> > > > > > > 
> > > > > > Its ok, this is the most accurate description from the previous threads on the
> > > > > > subject:
> > > > > > 2
> > > > > > 
> > > > > > The basic problem boils down the notion that some drivers, when they receive an
> > > > > > skb in their xmit paths, presume to have sole ownership of the skb, and as a
> > > > > > result may do things like add the skb to a list, or otherwise store stateful
> > > > > > data in the skb.  If the skb is shared, thats unsafe to do, as the stack still
> > > > > > holds a reference to the skb, and make make changes without serializing them
> > > > > > against the driver.  So we have to flag those drivers which preform these kinds
> > > > > > of actions.  xen-netfront doesn't strictly speaking modify any state directly ni
> > > > > > an skb, but it does place a pointer to the skb in a data structure here:
> > > > > > 
> > > > > > np->tx_skbs[id].skb = skb;
> > > > > > 
> > > > > > Which then gets handed off to the hypervisior.  Since the hypervisor now has
> > > > > > access to that skb pointer, and we can't be sure (from the guest perspective),
> > > > > > what it does with that information, it would be better to be safe by disallowing
> > > > > > shared skbs in this path.
> > > > > 
> > > > > The skb pointer itself doesn't get given to the backend/hypervisor. The
> > > > > page which skb->data refers to is granted to the backend domain, as are
> > > > > the pages in the frags.
> > > > > 
> > > > > I think we only need to be sure that the frontend doesn't rely on
> > > > > anything in the skb itself, right? Does skb->data or shinfo count from
> > > > > that perspective?
> > > > shinfo is definately a problem, as other devices may make modifications to it.
> > > > skb->data is probably safer, but is also potentially suspect (for instance if
> > > > another device appends an additional header to the data for instance)
> > > 
> > > A device is allowed to rely on these things being stable while in its
> > > start_xmit, right? (otherwise I don't see how any device can ever
> > > cope...).
> > > 
> > While the start_xmit routine is executing, yes.  Its only after the driver
> > returns, that it can have no expectation of an skb's data to remain stable.
> > 
> > > netfront only uses shinfo and ->data during start_xmit in order to
> > > create the necessary grant reference (which can be thought of as a DMA
> > > address passed to the virtual hardware). The only use of the stashed skb
> > > pointer outside of this are to dev_kfree_skb on tx completion (from
> > > either tx_buf_gc (normal completion) or release_tx_buf ("hardware"
> > > reset).
> > > 
> > Ok, if you're certain you can guarantee that the hypervisior makes no inspection
> > of the skb after the return from the driver, then you're safe
> 
> I believe this is the case, all that is exposed to the backend is the
> pfn, offset and length of the skb->data and frags at the time start_xmit
> was called.
> 
Ok, if you're sure, than this can be dropped.
Neil