The 9pfs Xen PV-device frontend has 2 issues regarding connection to
the backend. Fix them.
Juergen Gross (2):
9p/xen: fix version parsing
9p/xen: fix connection sequence
net/9p/trans_xen.c | 48 ++++++++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 17 deletions(-)
--
2.35.3
When connecting the Xen 9pfs frontend to the backend, the "versions"
Xenstore entry written by the backend is parsed in a wrong way.
The "versions" entry is defined to contain the versions supported by
the backend separated by commas (e.g. "1,2"). Today only version "1"
is defined. Unfortunately the frontend doesn't look for "1" being
listed in the entry, but it is expecting the entry to have the value
"1".
This will result in failure as soon as the backend will support e.g.
versions "1" and "2".
Fix that by scanning the entry correctly.
Fixes: 71ebd71921e4 ("xen/9pfs: connect to the backend")
Signed-off-by: Juergen Gross <[email protected]>
---
net/9p/trans_xen.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 82c7005ede65..ad2947a3b376 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -378,13 +378,19 @@ static int xen_9pfs_front_probe(struct xenbus_device *dev,
int ret, i;
struct xenbus_transaction xbt;
struct xen_9pfs_front_priv *priv = NULL;
- char *versions;
+ char *versions, *v;
unsigned int max_rings, max_ring_order, len = 0;
versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
if (IS_ERR(versions))
return PTR_ERR(versions);
- if (strcmp(versions, "1")) {
+ for (v = versions; *v; v++) {
+ if (simple_strtoul(v, &v, 10) == 1) {
+ v = NULL;
+ break;
+ }
+ }
+ if (v) {
kfree(versions);
return -EINVAL;
}
--
2.35.3
Today the connection sequence of the Xen 9pfs frontend doesn't match
the documented sequence. It can work reliably only for a PV 9pfs device
having been added at boot time already, as the frontend is not waiting
for the backend to have set its state to "XenbusStateInitWait" before
reading the backend properties from Xenstore.
Fix that by following the documented sequence [1] (the documentation
has a bug, so the reference is for the patch fixing that).
[1]: https://lore.kernel.org/xen-devel/[email protected]/T/#u
Fixes: 868eb122739a ("xen/9pfs: introduce Xen 9pfs transport driver")
Signed-off-by: Juergen Gross <[email protected]>
---
net/9p/trans_xen.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index ad2947a3b376..c64050e839ac 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -372,12 +372,11 @@ static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
return ret;
}
-static int xen_9pfs_front_probe(struct xenbus_device *dev,
- const struct xenbus_device_id *id)
+static int xen_9pfs_front_init(struct xenbus_device *dev)
{
int ret, i;
struct xenbus_transaction xbt;
- struct xen_9pfs_front_priv *priv = NULL;
+ struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev);
char *versions, *v;
unsigned int max_rings, max_ring_order, len = 0;
@@ -405,11 +404,6 @@ static int xen_9pfs_front_probe(struct xenbus_device *dev,
if (p9_xen_trans.maxsize > XEN_FLEX_RING_SIZE(max_ring_order))
p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order) / 2;
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
- priv->dev = dev;
priv->num_rings = XEN_9PFS_NUM_RINGS;
priv->rings = kcalloc(priv->num_rings, sizeof(*priv->rings),
GFP_KERNEL);
@@ -468,23 +462,35 @@ static int xen_9pfs_front_probe(struct xenbus_device *dev,
goto error;
}
- write_lock(&xen_9pfs_lock);
- list_add_tail(&priv->list, &xen_9pfs_devs);
- write_unlock(&xen_9pfs_lock);
- dev_set_drvdata(&dev->dev, priv);
- xenbus_switch_state(dev, XenbusStateInitialised);
-
return 0;
error_xenbus:
xenbus_transaction_end(xbt, 1);
xenbus_dev_fatal(dev, ret, "writing xenstore");
error:
- dev_set_drvdata(&dev->dev, NULL);
xen_9pfs_front_free(priv);
return ret;
}
+static int xen_9pfs_front_probe(struct xenbus_device *dev,
+ const struct xenbus_device_id *id)
+{
+ struct xen_9pfs_front_priv *priv = NULL;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = dev;
+ dev_set_drvdata(&dev->dev, priv);
+
+ write_lock(&xen_9pfs_lock);
+ list_add_tail(&priv->list, &xen_9pfs_devs);
+ write_unlock(&xen_9pfs_lock);
+
+ return 0;
+}
+
static int xen_9pfs_front_resume(struct xenbus_device *dev)
{
dev_warn(&dev->dev, "suspend/resume unsupported\n");
@@ -503,6 +509,8 @@ static void xen_9pfs_front_changed(struct xenbus_device *dev,
break;
case XenbusStateInitWait:
+ if (!xen_9pfs_front_init(dev))
+ xenbus_switch_state(dev, XenbusStateInitialised);
break;
case XenbusStateConnected:
--
2.35.3
On Mon, Jan 30, 2023 at 12:30:35PM +0100, Juergen Gross wrote:
> When connecting the Xen 9pfs frontend to the backend, the "versions"
> Xenstore entry written by the backend is parsed in a wrong way.
>
> The "versions" entry is defined to contain the versions supported by
> the backend separated by commas (e.g. "1,2"). Today only version "1"
> is defined. Unfortunately the frontend doesn't look for "1" being
> listed in the entry, but it is expecting the entry to have the value
> "1".
>
> This will result in failure as soon as the backend will support e.g.
> versions "1" and "2".
>
> Fix that by scanning the entry correctly.
>
> Fixes: 71ebd71921e4 ("xen/9pfs: connect to the backend")
> Signed-off-by: Juergen Gross <[email protected]>
It's unclear if this series is targeted at 'net' or 'net-next'.
FWIIW, I feel I feel it would be more appropriate for the latter
as these do not feel like bug fixes: feel free to differ on that.
Regardless,
Reviewed-by: Simon Horman <[email protected]>
On Mon, Jan 30, 2023 at 12:30:36PM +0100, Juergen Gross wrote:
> Today the connection sequence of the Xen 9pfs frontend doesn't match
> the documented sequence. It can work reliably only for a PV 9pfs device
> having been added at boot time already, as the frontend is not waiting
> for the backend to have set its state to "XenbusStateInitWait" before
> reading the backend properties from Xenstore.
>
> Fix that by following the documented sequence [1] (the documentation
> has a bug, so the reference is for the patch fixing that).
>
> [1]: https://lore.kernel.org/xen-devel/[email protected]/T/#u
>
> Fixes: 868eb122739a ("xen/9pfs: introduce Xen 9pfs transport driver")
> Signed-off-by: Juergen Gross <[email protected]>
It's unclear if this series is targeted at 'net' or 'net-next'.
FWIIW, I feel I feel it would be more appropriate for the latter
as these do not feel like bug fixes: feel free to differ on that.
Regardless,
Reviewed-by: Simon Horman <[email protected]>
On 31.01.23 19:48, Simon Horman wrote:
> On Mon, Jan 30, 2023 at 12:30:35PM +0100, Juergen Gross wrote:
>> When connecting the Xen 9pfs frontend to the backend, the "versions"
>> Xenstore entry written by the backend is parsed in a wrong way.
>>
>> The "versions" entry is defined to contain the versions supported by
>> the backend separated by commas (e.g. "1,2"). Today only version "1"
>> is defined. Unfortunately the frontend doesn't look for "1" being
>> listed in the entry, but it is expecting the entry to have the value
>> "1".
>>
>> This will result in failure as soon as the backend will support e.g.
>> versions "1" and "2".
>>
>> Fix that by scanning the entry correctly.
>>
>> Fixes: 71ebd71921e4 ("xen/9pfs: connect to the backend")
>> Signed-off-by: Juergen Gross <[email protected]>
>
> It's unclear if this series is targeted at 'net' or 'net-next'.
> FWIIW, I feel I feel it would be more appropriate for the latter
> as these do not feel like bug fixes: feel free to differ on that.
I'm fine with net-next.
Right now there is no problem with the current behavior. This will
change only in case Xen starts to support a new transport version.
For the other patch the problem would show up only if Xen starts
supporting dynamical attach/detach of 9pfs devices, which is not
the case right now.
>
> Regardless,
>
> Reviewed-by: Simon Horman <[email protected]>
>
Thanks,
Juergen
Juergen Gross wrote on Wed, Feb 01, 2023 at 07:37:04AM +0100:
> > It's unclear if this series is targeted at 'net' or 'net-next'.
> > FWIIW, I feel I feel it would be more appropriate for the latter
> > as these do not feel like bug fixes: feel free to differ on that.
>
> I'm fine with net-next.
It doesn't look like it got picked up in net-next, so I'm queueing it up in the
9p tree.
Thanks for the patches and the review!
--
Dominique