Re: [RAUC] Failed to mount seed slot

[Ladislav Michl <ladis@linux-mips.org>]

On Tue, May 07, 2019 at 05:59:36PM +0000, Trent Piepho wrote:
> On Tue, 2019-05-07 at 17:49 +0200, Ladislav Michl wrote:
> > On Tue, May 07, 2019 at 09:31:28AM +0200, Ladislav Michl wrote:
> > > On Mon, May 06, 2019 at 07:44:09PM +0200, Ladislav Michl wrote:
> > > > [slot.rootfs.0]
> > > > device=/dev/ubi0_0
> > > > type=ubifs
> > > > bootname=system0
> > > > 
> > > > Now system is booted with rootfs mounted read only which makes mounting seed
> > > > slot fail:
> > > > rauc[871]: Mounting /dev/ubi0_0 to use as seed
> > > > rauc[871]: launching subprocess: mount -t ubifs /dev/ubi0_0 /run/rauc/rootfs.0
> > > > rauc[871]: posix_spawn avoided (fd close requested) (child_setup specified) 
> > > > rauc[871]: mount: /run/rauc/rootfs.0: /dev/ubi0_0 already mounted or mount point busy.
> 
> What does the kernel show in /proc/mounts in this case?  If it's
> "/dev/ubi0_0", then it should match based on the simple path compare.

System is using barebox' bootchooser with boot loader specification. Here
device is ubi0:rootfs0. And yes, device name could be changed in rauc's
slot configuration to be ubi0:rootfs0 allowing path compare succeed.
But see bellow why it is worth changing code a bit.

> > Problem is similar to that one solved by #406 and #388, however it needs to be
> > extended to ubi<X> and possibly mtd<X> case. For now let's start with fixing
> > pull request #406 to:
> 
> Is there a way to mount a /dev/mtd<X> device directly, and not through
> /dev/mtdblock<X>?  I was under the impression that yaffs1/2 and jffs2
> used mtdblock, but I haven't used raw nand (what a PITA it is) in
> while.

I guess mtd layer does not need block device to mount for over a decade,
but haven't bothered to found that commit :)

> > - use C comments
> 
> Is this a requirement for rauc?  I see other // comments in rauc, they
> are part of the C standard since C99, and rauc's extensive github
> commit checking doesn't flag it.

I have no strong opinion here, just made it consistent with the rest
of file.

> > - avoid unneded allocations
> 
> Perhaps this is better, since none of the allocations need to live.
> 
> > - also allow character devices (for example /dev/ubi0_0)
> 
> Seems reasonable to fix that case.  This would work for /dev/mtd too,
> if there is a way to mount those.

Please see http://www.linux-mtd.infradead.org/faq/jffs2.html#L_mtdblock

> > > Then determine_slot_states should properly fill slot's ext_mount_point and
> > > casync_extract_image would then use it to bind mount seed slot.
> > > 
> > > And here's the tricky part. For device-less mount, volume to mount is specified
> > > for example using ubiX_Y or ubiX:NAME syntax, so fix in #406 will fail in this
> > > case.
> 
> If the mount call uses ubiX:NAME, what does /proc/mounts report?  That
> really the issue: matching the config file device string to the string
> reported by the kernel when the device is mounted.

/proc/mounts reports whatever was used as mount argument.

> > This one was fixed by extending normalize_mountable_object from #406 and
> > adjusting casync_extract_image, but lets first get #406 revieved and merged :)
> > Trent, in case you want to merge following additional patch to your PR here's my
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > 
> > diff --git a/src/config_file.c b/src/config_file.c
> > index 875e0ee..e592d15 100644
> > --- a/src/config_file.c
> > +++ b/src/config_file.c
> > @@ -494,27 +494,25 @@ free:
> >  	return res;
> >  }
> >  
> > -// Something that is, or can be, mounted onto a mount point
> > +/* Something that is, or can be, mounted onto a mount point */
> >  typedef struct {
> > -	gboolean is_device;  // vs being a loop mounted file
> > -	dev_t dev;  // the device, or for a file, the device the file is on
> > -	ino_t inode;  // inode of file for a non-device
> > +	gboolean is_device;	/* vs being a loop mounted file */
> > +	dev_t dev;		/* the device, or for a file, the device the file is on */
> > +	ino_t inode;		/* inode of file for a non-device */
> >  } MountableObj;
> 
> There would also need to be another bool to indicate if the device is a
>  block dev or char dev, since the major:minor could match between
> different device types.  That could be part of a union with inode since
> the two are mutually exclusive.

Ah, will fix that. Eventually it is possible to walk /sys/dev using major:minor
and find corresponding uevent file which is holding DEVNAME key, but I like
your approach a lot more as it will also allow us to move all logic which is
now scattered over different places to find_slot_by_device. The loop in
determine_slot_states then boils down to:

	/* Determine active slot mount points */
	mountlist = g_unix_mounts_get(NULL);
	for (GList *l = mountlist; l != NULL; l = l->next) {
		GUnixMountEntry *m = (GUnixMountEntry*)l->data;
		RaucSlot *s = find_config_slot_by_device(r_context()->config, g_unix_mount_get_device_path(m));
		if (!s || s->ext_mount_point)
			continue;

		s->ext_mount_point = g_strdup(g_unix_mount_get_mount_path(m));
		g_debug("Found external mountpoint for slot %s at %s",
				s->name, s->ext_mount_point);
	}
	g_list_free_full(mountlist, (GDestroyNotify)g_unix_mount_free);

Then we can remove resolve_loop_device as your match code should cover
that special case too and with normalize_mountable_object first checking
for "special" device names and then stating path if it is starting with
'/' all quirks elsewhere are gone.

So here's the last remaining piece to puzzle:
--- a/src/update_handler.c
+++ b/src/update_handler.c
@@ -253,11 +253,29 @@ static gboolean casync_extract_image(RaucImage *image, gchar *dest, GError **err
 		 * file systems, additional mounts, etc. */
 		if (!seedslot->mount_point) {
 			g_debug("Mounting %s to use as seed", seedslot->device);
-			res = r_mount_slot(seedslot, &ierror);
-			if (!res) {
-				g_warning("Failed mounting for seeding: %s", ierror->message);
-				g_clear_error(&ierror);
-				goto extract;
+			if (seedslot->ext_mount_point) {
+				gchar *mount_point = r_create_mount_point(seedslot->name, &ierror);
+				if (!mount_point) {
+					g_warning("Failed creating bind mount point for seeding: %s", ierror->message);
+					g_clear_error(&ierror);
+					goto extract;
+				}
+				res = r_mount_full(seedslot->ext_mount_point, mount_point, NULL, 0, "bind", &ierror);
+				if (!res) {
+					g_warning("Failed bind mounting for seeding: %s", ierror->message);
+					g_clear_error(&ierror);
+					g_rmdir(mount_point);
+					g_free(mount_point);
+					goto extract;
+				}
+				seedslot->mount_point = mount_point;
+			} else {
+				res = r_mount_slot(seedslot, &ierror);
+				if (!res) {
+					g_warning("Failed mounting for seeding: %s", ierror->message);
+					g_clear_error(&ierror);
+					goto extract;
+				}
 			}
 			seed_mounted = TRUE;
 		}

Above change would actually bind mount slot for seeding...

> > -// Take a device (or file) path and normalize it
> > -static MountableObj *normalize_mountable_object(const gchar *devicepath)
> > +/* Take a device (or file) path or name and normalize it */
> > +static gboolean normalize_mountable_object(const gchar *devicename, MountableObj *obj)
> >  {
> > -	MountableObj *obj;
> >  	GStatBuf st;
> >  
> > -	if (g_stat(devicepath, &st) == -1) {
> > +	if (g_stat(devicename, &st) == -1) {
> >  		/* Virtual filesystems like devpts trigger case */
> > -		g_debug("Can't stat '%s', assuming unmountable: %s", devicepath, g_strerror(errno));
> > -		return NULL;
> > +		g_debug("Can't stat '%s', assuming unmountable: %s", devicename, g_strerror(errno));
> > +		return FALSE;
> >  	}
> >  
> > -	obj = g_new0(MountableObj, 1);
> > -	if (S_ISBLK(st.st_mode)) {
> > +	if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) {
> >  		obj->is_device = TRUE;
> >  		obj->dev = st.st_rdev;
> >  	} else if (S_ISREG(st.st_mode)) {
> > @@ -522,10 +520,10 @@ static MountableObj *normalize_mountable_object(const gchar *devicepath)
> >  		obj->dev = st.st_dev;
> >  		obj->inode = st.st_ino;
> >  	} else {
> > -		g_debug("Device '%s' is not something which is mountable", devicepath);
> > -		g_clear_pointer(&obj, g_free);
> > +		g_debug("Device '%s' is not something which is mountable", devicename);
> > +		return FALSE;
> >  	}
> > -	return obj;
> > +	return TRUE;
> >  }
> >  
> >  /* Compare two MountableObj for equality */
> > @@ -546,24 +544,24 @@ RaucSlot *find_slot_by_device(GHashTable *slots, const gchar *device)
> >  {
> >  	GHashTableIter iter;
> >  	RaucSlot *slot;
> > -	g_autofree MountableObj *obj = NULL;
> > +	MountableObj obj;
> > +	gboolean normalized;
> >  
> >  	g_return_val_if_fail(slots, NULL);
> >  	g_return_val_if_fail(device, NULL);
> >  
> > -	obj = normalize_mountable_object(device);
> > +	normalized = normalize_mountable_object(device, &obj);
> >  
> >  	g_hash_table_iter_init(&iter, slots);
> >  	while (g_hash_table_iter_next(&iter, NULL, (gpointer*) &slot)) {
> >  		if (g_strcmp0(slot->device, device) == 0)
> >  			goto out;
> >  
> > -		// Path doesn't match, but maybe device is same?
> > -		if (obj) {
> > -			g_autofree MountableObj *slot_obj =
> > -				normalize_mountable_object(slot->device);
> > -
> > -			if (slot_obj && is_same_mountable_object(obj, slot_obj))
> > +		/* Path doesn't match, but maybe device is the same? */
> > +		if (normalized) {
> > +			MountableObj slot_obj;
> > +			if (normalize_mountable_object(slot->device, &slot_obj) &&
> > +			    is_same_mountable_object(&obj, &slot_obj))
> >  				goto out;
> >  		}
> >  	}

_______________________________________________
RAUC mailing list