From: Samuel Thibault <samuel.thibault@ens-lyon.org>
To: Juergen Gross <jgross@suse.com>
Cc: minios-devel@lists.xenproject.org,
xen-devel@lists.xenproject.org, wl@xen.org
Subject: Re: [PATCH 2/2] mini-os: netfront: fix suspend/resume handling
Date: Tue, 22 Sep 2020 22:08:28 +0200 [thread overview]
Message-ID: <20200922200828.lh22kzmlgktbooma@function> (raw)
In-Reply-To: <20200922105826.26274-3-jgross@suse.com>
Juergen Gross, le mar. 22 sept. 2020 12:58:26 +0200, a ecrit:
> Suspend/resume handling of netfront is completely broken from the
> beginning. Commit d225f4012d69a1 ("Save/Restore Support: Add
> suspend/restore support for netfront") introduced a new structure
> netfront_dev_list referencing the real struct netfront_dev elements.
> This list is used to setup the devices when resuming again.
>
> Unfortunately the netfront_dev elements are released during suspend,
> so at resume time those references will be stale.
>
> Fix this whole mess by dropping struct netfront_dev_list again and
> link the netfront_dev elements directly into a list. When suspending
> don't free those elements.
>
> The ip-address, netmask and gateway strings can just be released when
> suspending and reread from xenstore at resume time.
>
> Fixes: d225f4012d69a1 ("Save/Restore Support: Add suspend/restore support for netfront")
> Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> netfront.c | 162 ++++++++++++++++++++++-------------------------------
> 1 file changed, 67 insertions(+), 95 deletions(-)
>
> diff --git a/netfront.c b/netfront.c
> index 9057908..2075410 100644
> --- a/netfront.c
> +++ b/netfront.c
> @@ -36,6 +36,8 @@ struct net_buffer {
> };
>
> struct netfront_dev {
> + int refcount;
> +
> domid_t dom;
>
> unsigned short tx_freelist[NET_TX_RING_SIZE + 1];
> @@ -66,27 +68,19 @@ struct netfront_dev {
> void (*netif_rx)(unsigned char* data, int len, void* arg);
> void *netif_rx_arg;
>
> - struct netfront_dev_list *ldev;
> -};
> -
> -struct netfront_dev_list {
> - struct netfront_dev *dev;
> unsigned char rawmac[6];
> char *ip;
> char *mask;
> char *gw;
>
> - int refcount;
> -
> - struct netfront_dev_list *next;
> + struct netfront_dev *next;
> };
>
> -static struct netfront_dev_list *dev_list = NULL;
> +static struct netfront_dev *dev_list = NULL;
>
> void init_rx_buffers(struct netfront_dev *dev);
> -static struct netfront_dev *_init_netfront(struct netfront_dev *dev,
> - unsigned char rawmac[6], char **ip, char **mask, char **gw);
> -static void _shutdown_netfront(struct netfront_dev *dev);
> +static struct netfront_dev *_init_netfront(struct netfront_dev *dev);
> +static int _shutdown_netfront(struct netfront_dev *dev);
> void netfront_set_rx_handler(struct netfront_dev *dev,
> void (*thenetif_rx)(unsigned char *data, int len,
> void *arg),
> @@ -276,6 +270,7 @@ static void free_netfront(struct netfront_dev *dev)
> mask_evtchn(dev->evtchn);
>
> free(dev->mac);
> + free(dev->ip);
> free(dev->backend);
>
> gnttab_end_access(dev->rx_ring_ref);
> @@ -309,8 +304,7 @@ struct netfront_dev *init_netfront(char *_nodename,
> {
> char nodename[256];
> struct netfront_dev *dev;
> - struct netfront_dev_list *ldev = NULL;
> - struct netfront_dev_list *list = NULL;
> + struct netfront_dev *list;
> static int netfrontends = 0;
>
> if (!_nodename)
> @@ -321,10 +315,9 @@ struct netfront_dev *init_netfront(char *_nodename,
> }
>
> /* Check if the device is already initialized */
> - for (list = dev_list; list != NULL; list = list->next) {
> - if (strcmp(nodename, list->dev->nodename) == 0) {
> - list->refcount++;
> - dev = list->dev;
> + for (dev = dev_list; dev != NULL; dev = dev->next) {
> + if (strcmp(nodename, dev->nodename) == 0) {
> + dev->refcount++;
> if (thenetif_rx)
> netfront_set_rx_handler(dev, thenetif_rx, NULL);
> goto out;
> @@ -345,40 +338,34 @@ struct netfront_dev *init_netfront(char *_nodename,
> dev->netif_rx = thenetif_rx;
> dev->netif_rx_arg = NULL;
>
> - ldev = malloc(sizeof(struct netfront_dev_list));
> - memset(ldev, 0, sizeof(struct netfront_dev_list));
> -
> - if (_init_netfront(dev, ldev->rawmac, &(ldev->ip), &(ldev->mask), &(ldev->gw))) {
> - dev->ldev = ldev;
> - ldev->dev = dev;
> - ldev->refcount = 1;
> - ldev->next = NULL;
> + if (_init_netfront(dev)) {
> + dev->refcount = 1;
> + dev->next = NULL;
>
> if (!dev_list) {
> - dev_list = ldev;
> + dev_list = dev;
> } else {
> for (list = dev_list; list->next != NULL; list = list->next)
> ;
> - list->next = ldev;
> - }
> + list->next = dev;
> + }
> netfrontends++;
> } else {
> - free(ldev);
> dev = NULL;
> goto err;
> }
>
> out:
> if (rawmac) {
> - rawmac[0] = ldev->rawmac[0];
> - rawmac[1] = ldev->rawmac[1];
> - rawmac[2] = ldev->rawmac[2];
> - rawmac[3] = ldev->rawmac[3];
> - rawmac[4] = ldev->rawmac[4];
> - rawmac[5] = ldev->rawmac[5];
> + rawmac[0] = dev->rawmac[0];
> + rawmac[1] = dev->rawmac[1];
> + rawmac[2] = dev->rawmac[2];
> + rawmac[3] = dev->rawmac[3];
> + rawmac[4] = dev->rawmac[4];
> + rawmac[5] = dev->rawmac[5];
> }
> if (ip)
> - *ip = strdup(ldev->ip);
> + *ip = strdup(dev->ip);
>
> err:
> return dev;
> @@ -386,17 +373,15 @@ err:
>
> char *netfront_get_netmask(struct netfront_dev *dev)
> {
> - return dev->ldev->mask ? strdup(dev->ldev->mask) : NULL;
> + return dev->mask ? strdup(dev->mask) : NULL;
> }
>
> char *netfront_get_gateway(struct netfront_dev *dev)
> {
> - return dev->ldev->gw ? strdup(dev->ldev->gw) : NULL;
> + return dev->gw ? strdup(dev->gw) : NULL;
> }
>
> -static struct netfront_dev *_init_netfront(struct netfront_dev *dev,
> - unsigned char rawmac[6],
> - char **ip, char **mask, char **gw)
> +static struct netfront_dev *_init_netfront(struct netfront_dev *dev)
> {
> xenbus_transaction_t xbt;
> char* err = NULL;
> @@ -518,6 +503,8 @@ done:
> {
> XenbusState state;
> char path[strlen(dev->backend) + strlen("/state") + 1];
> + char *p;
> +
> snprintf(path, sizeof(path), "%s/state", dev->backend);
>
> xenbus_watch_path_token(XBT_NIL, path, path, &dev->events);
> @@ -532,26 +519,18 @@ done:
> goto error;
> }
>
> - if (ip) {
> - char *p;
> -
> - snprintf(path, sizeof(path), "%s/ip", dev->backend);
> - xenbus_read(XBT_NIL, path, ip);
> -
> - if (mask) {
> - p = strchr(*ip, ' ');
> - if (p) {
> - *p++ = '\0';
> - *mask = p;
> -
> - if (gw) {
> - p = strchr(p, ' ');
> - if (p) {
> - *p++ = '\0';
> - *gw = p;
> - }
> - }
> - }
> + snprintf(path, sizeof(path), "%s/ip", dev->backend);
> + xenbus_read(XBT_NIL, path, &dev->ip);
> +
> + p = strchr(dev->ip, ' ');
> + if (p) {
> + *p++ = '\0';
> + dev->mask = p;
> +
> + p = strchr(p, ' ');
> + if (p) {
> + *p++ = '\0';
> + dev->gw = p;
> }
> }
> }
> @@ -563,14 +542,13 @@ done:
> /* Special conversion specifier 'hh' needed for __ia64__. Without
> * this mini-os panics with 'Unaligned reference'.
> */
> - if (rawmac)
> - sscanf(dev->mac,"%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
> - &rawmac[0],
> - &rawmac[1],
> - &rawmac[2],
> - &rawmac[3],
> - &rawmac[4],
> - &rawmac[5]);
> + sscanf(dev->mac,"%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
> + &dev->rawmac[0],
> + &dev->rawmac[1],
> + &dev->rawmac[2],
> + &dev->rawmac[3],
> + &dev->rawmac[4],
> + &dev->rawmac[5]);
>
> return dev;
>
> @@ -600,38 +578,33 @@ int netfront_tap_open(char *nodename) {
>
> void shutdown_netfront(struct netfront_dev *dev)
> {
> - struct netfront_dev_list *list = NULL;
> - struct netfront_dev_list *to_del = NULL;
> + struct netfront_dev *list;
>
> /* Check this is a valid device */
> - for (list = dev_list; list != NULL; list = list->next) {
> - if (list->dev == dev)
> - break;
> - }
> + for (list = dev_list; list != NULL && list != dev; list = list->next);
>
> if (!list) {
> printk("Trying to shutdown an invalid netfront device (%p)\n", dev);
> return;
> }
>
> - list->refcount--;
> - if (list->refcount == 0) {
> - _shutdown_netfront(dev);
> + dev->refcount--;
> + if (dev->refcount == 0) {
> + if (_shutdown_netfront(dev))
> + return;
>
> - to_del = list;
> - if (to_del == dev_list) {
> - free(to_del);
> - dev_list = NULL;
> + if (dev == dev_list) {
> + dev_list = NULL;
> } else {
> - for (list = dev_list; list->next != to_del; list = list->next)
> + for (list = dev_list; list->next != dev; list = list->next)
> ;
> - list->next = to_del->next;
> - free(to_del);
> + list->next = dev->next;
> }
> + free_netfront(dev);
> }
> }
>
> -static void _shutdown_netfront(struct netfront_dev *dev)
> +static int _shutdown_netfront(struct netfront_dev *dev)
> {
> char* err = NULL, *err2;
> XenbusState state;
> @@ -692,24 +665,23 @@ close:
> err2 = xenbus_rm(XBT_NIL, nodename);
> free(err2);
>
> - if (!err)
> - free_netfront(dev);
> + return err ? -EBUSY : 0;
> }
>
> void suspend_netfront(void)
> {
> - struct netfront_dev_list *list;
> + struct netfront_dev *dev;
>
> - for (list = dev_list; list != NULL; list = list->next)
> - _shutdown_netfront(list->dev);
> + for (dev = dev_list; dev != NULL; dev = dev->next)
> + _shutdown_netfront(dev);
> }
>
> void resume_netfront(void)
> {
> - struct netfront_dev_list *list;
> + struct netfront_dev *dev;
>
> - for (list = dev_list; list != NULL; list = list->next)
> - _init_netfront(list->dev, NULL, NULL, NULL, NULL);
> + for (dev = dev_list; dev != NULL; dev = dev->next)
> + _init_netfront(dev);
> }
>
> void init_rx_buffers(struct netfront_dev *dev)
> --
> 2.26.2
>
--
Samuel
tohi.cybercable.fr (212.198.0.3) si une personne se reconnait derriere
cette adresse que ce soit un pirate ou une victime qu'il se manifeste,
cette personne pourrait bien etre un petit malin
-+- Fred in NPC : Mamaaaaan, y a le routeur qui veut me hacker -+-
next prev parent reply other threads:[~2020-09-22 20:11 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-22 10:58 [PATCH 0/2] mini-os: netfront: fix some issues Juergen Gross
2020-09-22 10:58 ` [PATCH 1/2] mini-os: netfront: retrieve netmask and gateway via extra function Juergen Gross
2020-09-22 20:08 ` Samuel Thibault
2020-09-22 10:58 ` [PATCH 2/2] mini-os: netfront: fix suspend/resume handling Juergen Gross
2020-09-22 20:08 ` Samuel Thibault [this message]
2020-10-01 9:37 ` [PATCH 0/2] mini-os: netfront: fix some issues Wei Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200922200828.lh22kzmlgktbooma@function \
--to=samuel.thibault@ens-lyon.org \
--cc=jgross@suse.com \
--cc=minios-devel@lists.xenproject.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.