| Message ID | 1321298544-16434-1-git-send-email-nhorman@tuxdriver.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 :-) -- 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
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 > -- 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
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; > -- 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
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 > -- 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
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 > > -- 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
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 -- 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
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. -- 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
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. > > -- 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
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. > > > > -- 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
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 -- 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
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;
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(-)