Subject: Re: CONFIG_VFAT_FS_DUALNAMES regressions - msg#01314

Previous Message by Date: click to view message preview

Re: CONFIG_VFAT_FS_DUALNAMES regression

Hi Jan, > Yes, here is your precedent case. The DUALNAMES patch breaks the > operation on my Fujifilm Finepix A210 digital camera. interesting, but I think you may have been trapped by differences in the VFAT mounting options between the two examples. Would you mind retesting again, but looking carefully at the VFAT mount options. I don't think what you've described can happen without differing VFAT options between your two examples. I'll try and expain: > Here, dscf4159.jpg is a preexisting file created by the camera itself > (and subsequently, Finepix's FAT code) as a result of taking a picture. > Then I just copied that file with two different kernels in succession. ok, dscf4159.jpg may or may not be a 8.3 name depending on the "shortname=" mount option. As this is a digital camera, I suspect that it was created as an 8.3 name, although you'd have to confirm that by inspecting the disk image. If you made a copy of the image available to me somewhere (please don't email it unless it compresses to a small size!) then I can tell you what the camera did in this case. > * 2.6.31-rc1 + dualnames=n > # mount /dev/sdc1 /mnt > # cd /mnt/dcim/100_fuji/; cp dscf4159.jpg dscf3000.jpg > # umount /mnt Now we would need to know what shortname= option was used here. If the shortname= option was such that the lowercase name dscf3000.jpg was considered as 8.3, then the patch I have posted would have had zero effect on what was stored on disk. If it was not considered a short name (because you have the shortname= option set to not allow lowercase short names) then it would have been created as a long name. As you said the file became invisible then I suspect that for this test you had the shortname= option set to treat lowercase names as not being allowed in 8.3. > * 2.6.29.5 w/o patch > # same procedure > cp dscf4159.jpg dscf3001.jpg > > End result is that picture ID 3000 is not selectable in the camera's > built-in menu (OSD), as if the file did not exist. 3001 was shown, > however. ok, if 3001 was shown, then in this case you must have had different VFAT mount options. I don't know if this is because the different base kernel has different defaults, or if perhaps you used a different system that had different mounting options. If you had used the same mount options as the previous test, then the 3001 name could not have shown up I think. It would have shown up as something like DSCF3~01.JPG. So I think all you've found is two things we already knew: 1) VFAT mount options make for an easy trap to fall into :-) 2) as the config text in the patch explains: "That means that long filenames created with this option disabled will not be accessible at all to operating systems that do not understand the VFAT extensions." If you mount with the right shortname= options then I think you'll find that your above test will work (for some value of work!) But please do keep testing. We want to eliminate the possibility that you have found a real example of where the patch causes a problem with some embedded systems. Cheers, Tridge -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/

Next Message by Date: click to view message preview

Re: [PATCH 1/2] Adds a read-only "procs" file similar to "tasks" that shows only unique tgids

On Thu, 02 Jul 2009 16:26:20 -0700 Paul Menage <menage@xxxxxxxxxx> wrote: > Adds a read-only "procs" file similar to "tasks" that shows only unique tgids > > struct cgroup used to have a bunch of fields for keeping track of the pidlist > for the tasks file. Those are now separated into a new struct cgroup_pidlist, > of which two are had, one for procs and one for tasks. The way the seq_file > operations are set up is changed so that just the pidlist struct gets passed > around as the private data. > > Possible future functionality is making the procs file writable for purposes > of adding all threads with the same tgid at once. > It'd be nice were the changelog to spell out the contents of this file (via an example) so we can review the proposed userspace interface change. > --- > > include/linux/cgroup.h | 23 +++- > kernel/cgroup.c | 287 > ++++++++++++++++++++++++++++++------------------ It'd be nice if the proposed userspace interface were documented too. > > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h > index 665fa70..3b937c3 100644 > --- a/include/linux/cgroup.h > +++ b/include/linux/cgroup.h > > ... > > -/* > - * Load into 'pidarray' up to 'npids' of the tasks using cgroup > - * 'cgrp'. Return actual number of pids loaded. No need to > - * task_lock(p) when reading out p->cgroup, since we're in an RCU > - * read section, so the css_set can't go away, and is > - * immutable after creation. > +/** > + * pidlist_uniq - given a kmalloc()ed list, strip out all duplicate entries > + * returns -ENOMEM if the malloc fails; 0 on success > + */ The comment purports to be kerneldoc ("/**") but didn't document the function's arguments. > +/* this implementation relies on the fact that pids will never be -1 */ > +#define PIDLIST_VALUE_NONE ((pid_t)(-1)) > +static int pidlist_uniq(pid_t **p, int *length) > +{ > + int i, j = 0; /* array index */ > + int last = 0; /* index of last unique entry */ > + int count = 1; /* count of total unique entries */ > + pid_t *list, *newlist; a newline here is conventional. > + BUG_ON(p == NULL || *p == NULL || length == NULL); Unneeded, really. The kernel will reliably oops if any of these are true, which provides us with the same info. In fact debuggability would be enhanced were this assertion to be removed, because if this goes blam then it will be very hard to work out which of these three tests went wrong. > + list = *p; > + /* > + * we presume the 0th element is unique, so i starts at 1. trivial > + * edge cases first; no work needs to be done for either > + */ > + if (*length == 0 || *length == 1) > + return 0; > + for (i = 1; i < *length; i++) { > + BUG_ON(list[i] == PIDLIST_VALUE_NONE); > + if (list[i] == list[last]) { > + list[i] = PIDLIST_VALUE_NONE; > + } else { > + last = i; > + count++; > + } > + } > + newlist = kmalloc(count * sizeof(pid_t), GFP_KERNEL); What is the maximum possible value of `count' here? Is there any way in which malicious code can exploit the potential multiplicative overflow in this statement? (kcalloc() checks for this). > + if (!newlist) > + return -ENOMEM; > + /* copy to new array */ > + for (i = 0; i < *length; i++) { > + if (list[i] != PIDLIST_VALUE_NONE) { > + newlist[j] = list[i]; > + j++; > + } > + } > + BUG_ON(j != count); /* this would fail on a zero-length array */ > + kfree(list); > + *p = newlist; > + *length = count; > + return 0; > +} > + > +static int cmppid(const void *a, const void *b) > +{ > + return *(pid_t *)a - *(pid_t *)b; > +} > + > +/** > + * Load a cgroup's pidarray with either procs' tgids or tasks' pids > */ again, that ain't kerneldoc. > -static int pid_array_load(pid_t *pidarray, int npids, struct cgroup *cgrp) > +static int pidlist_array_load(struct cgroup *cgrp, bool procs) > { > - int n = 0, pid; > + pid_t *array; > + int length; > + int retval; > + int pid, n = 0; /* used for populating the array */ > struct cgroup_iter it; > struct task_struct *tsk; > + struct cgroup_pidlist *l; > + > + /* > + * If cgroup gets more users after we read count, we won't have > + * enough space - tough. This race is indistinguishable to the > + * caller from the case that the additional cgroup users didn't > + * show up until sometime later on. > + */ > + length = cgroup_task_count(cgrp); > + array = kmalloc(length * sizeof(pid_t), GFP_KERNEL); max size? overflowable? > + if (!array) > + return -ENOMEM; > + /* now, populate the array */ > cgroup_iter_start(cgrp, &it); > while ((tsk = cgroup_iter_next(cgrp, &it))) { > - if (unlikely(n == npids)) > + if (unlikely(n == length)) > break; > - pid = task_pid_vnr(tsk); > - if (pid > 0) > - pidarray[n++] = pid; > + /* get tgid or pid for procs or tasks file respectively */ > + pid = (procs ? task_tgid_vnr(tsk) : task_pid_vnr(tsk)); > + if (pid > 0) /* make sure to only use valid results */ > + array[n++] = pid; > } > cgroup_iter_end(cgrp, &it); > - return n; > + length = n; > + /* now sort & (if procs) strip out duplicates */ > + sort(array, length, sizeof(pid_t), cmppid, NULL); > + if (procs) { > + retval = pidlist_uniq(&array, &length); > + if (retval) { /* the malloc inside uniq can fail */ > + kfree(array); > + return retval; > + } > + l = &(cgrp->procs); > + } else { > + l = &(cgrp->tasks); > + } > + /* store array in cgroup, freeing old if necessary */ > + down_write(&l->mutex); > + kfree(l->list); > + l->list = array; > + l->length = length; > + l->use_count++; > + up_write(&l->mutex); > + return 0; > } > > + > /** > * cgroupstats_build - build and fill cgroupstats > * @stats: cgroupstats to fill information into > > ... > > -static int cgroup_tasks_release(struct inode *inode, struct file *file) > +static int cgroup_pidlist_release(struct inode *inode, struct file *file) > { > - struct cgroup *cgrp = __d_cgrp(file->f_dentry->d_parent); > - > + struct cgroup_pidlist *l; > if (!(file->f_mode & FMODE_READ)) > return 0; > - > - release_cgroup_pid_array(cgrp); > + /* > + * the seq_file will only be initialized if the file was opened for > + * reading; hence we check if it's not null only in that case. > + */ > + BUG_ON(!file->private_data); Unneeded assertion for reasons described above. > + l = ((struct seq_file *)file->private_data)->private; > + cgroup_release_pid_array(l); > return seq_release(inode, file); > } > > > ... > > @@ -2389,21 +2457,27 @@ static int cgroup_write_notify_on_release(struct > cgroup *cgrp, > /* > * for the common functions, 'private' gives the type of file > */ > +/* for hysterical reasons, we can't put this on the older files */ "raisins" ;) > +#define CGROUP_FILE_GENERIC_PREFIX "cgroup." > static struct cftype files[] = { > { > .name = "tasks", > .open = cgroup_tasks_open, > .write_u64 = cgroup_tasks_write, > - .release = cgroup_tasks_release, > - .private = FILE_TASKLIST, > + .release = cgroup_pidlist_release, > .mode = S_IRUGO | S_IWUSR, > }, > - > + { > + .name = CGROUP_FILE_GENERIC_PREFIX "procs", > + .open = cgroup_procs_open, > + /* .write_u64 = cgroup_procs_write, TODO */ > + .release = cgroup_pidlist_release, > + .mode = S_IRUGO, > + }, > { > .name = "notify_on_release", > .read_u64 = cgroup_read_notify_on_release, > .write_u64 = cgroup_write_notify_on_release, > - .private = FILE_NOTIFY_ON_RELEASE, > }, > }; > > > ... > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/

Previous Message by Thread: click to view message preview

Re: CONFIG_VFAT_FS_DUALNAMES regression

Jan Engelhardt wrote: > > On Friday 2009-07-03 03:26, tridge@xxxxxxxxx wrote: > > > > > Making WINNT the default would cause many a `ls` output to just > > > scream at me in uppercaps because there are programs that > > > create long names with all-uppers. > > > >well, you could also argue that having WINNT in effect does the > >'correct' thing. It causes ls to display the name that is actually in > >the filesystem. > > > >I think the current default for VFAT on Linux is rather misleading. It > >always displays 8.3 names as lowercase, > > There is no misleading in that, since VFAT is rather case-insensitive. It's case-insensitive for searches, but: - With the "posix" option, you can have multiple names with different cases. - It still preserves case. I've got (Linux!) devices which copy files from FAT flash media onto their internal hard disk which is ext3, and then fail to work if the files on ext3 have the wrong case. -- Jamie -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/

Next Message by Thread: click to view message preview

Re: CONFIG_VFAT_FS_DUALNAMES regressions

Hi Jan, I should also mention that in the first patch I sent back in May, I added some code that made the 8.3 name problem easier on the user. It basically forced the shortname= option, overriding the mount option, when the patch was triggered. The end result was that a name like dcis3000.jpg would always be stored as a 8.3 name, and thus be visible to digital cameras, even if the mount options were set to say that lowercase names should be considered as "long" names. Hirofumi objected, on the basis that I was overriding the VFAT mount option. I think that overriding in this case would in fact be worthwhile, as I don't think many users ever think carefully about what shortname= option they are using, and unless you get it right then you can get some real surprises. Hirofumi-san, do you think that same objection you expressed previously also applies to this new patch? For example, would you think it reasonable to modify the patch to store the filename as 8.3 if it can fit in 8.3 in a case mapped manner? Or store as 8.3 if it is all uppercase or all lowercase? I think we will hit cases like the one Jan has pointed out where the exising shortname= options will become even more confusing than they were already when this new patch is applied, especially as I think most distros these days magically supply a shortname= option when you insert some VFAT formatted media. Cheers, Tridge -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/