* Re: kernel 2.4.27-10: isofs driver ignore some parameters with mount [not found] <1853917171.20050812104417@tut.by> @ 2005-08-12 8:29 ` Horms 2005-08-12 8:41 ` Horms 2005-08-16 1:11 ` Marcelo Tosatti 0 siblings, 2 replies; 6+ messages in thread From: Horms @ 2005-08-12 8:29 UTC (permalink / raw) To: Alexander Pytlev; +Cc: Marcelo Tosatti, linux-kernel, debian-kernel On Fri, Aug 12, 2005 at 10:44:17AM +0300, Alexander Pytlev wrote: > Hello Debian, > > Kernel 2.4.27-10 > With mount isofs filesystem, any mount parameters after > iocharset=,map=,session= are ignored. > > Sample: > > mount -t isofs -o uid=100,iocharset=koi8-r,gid=100 /dev/cdrom /media/cdrom > > gid=100 - was ignored > > I look in source and find that problem. I make two patch, simply and full > (what addeded some functionality - ignore wrong mount parameters) Thanks, I will try and get the simple version of this patch into the next Sarge update. I have also CCed Marcelo and the LKML for their consideration, as this problem still seems to be present in the lastest 2.4 tree. -- Horms simply patch: =================================================================================== --- kernel-source-2.4.27/fs/isofs/inode.c 2005-05-19 13:29:39.000000000 +0300 +++ kernel-source/fs/isofs/inode.c 2005-08-11 11:55:12.000000000 +0300 @@ -340,13 +340,13 @@ else if (!strcmp(value,"acorn")) popt->map = 'a'; else return 0; } - if (!strcmp(this_char,"session") && value) { + else if (!strcmp(this_char,"session") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >99) return 0; popt->session=ivalue+1; } - if (!strcmp(this_char,"sbsector") && value) { + else if (!strcmp(this_char,"sbsector") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >660*512) return 0; =================================================================================== full patch: =================================================================================== --- kernel-source-2.4.27/fs/isofs/inode.c 2005-05-19 13:29:39.000000000 +0300 +++ kernel-source/fs/isofs/inode.c 2005-08-11 11:50:56.000000000 +0300 @@ -327,10 +327,11 @@ popt->iocharset = value; while (*value && *value != ',') value++; - if (value == popt->iocharset) - return 0; *value = 0; - } else + if (value == popt->iocharset){ + printk("Invalid or missed parameter:%s=%s,\n",this_char,value); + } + } #endif if (!strcmp(this_char,"map") && value) { if (value[0] && !value[1] && strchr("ano",*value)) @@ -338,28 +339,30 @@ else if (!strcmp(value,"off")) popt->map = 'o'; else if (!strcmp(value,"normal")) popt->map = 'n'; else if (!strcmp(value,"acorn")) popt->map = 'a'; - else return 0; + else printk("Invalid or missed parameter:%s=%s,\n",this_char,value); } if (!strcmp(this_char,"session") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); - if(ivalue < 0 || ivalue >99) return 0; - popt->session=ivalue+1; + if(ivalue < 0 || ivalue >99) + printk("Invalid or missed parameter:%s=%s,\n",this_char,value); + else popt->session=ivalue+1; } if (!strcmp(this_char,"sbsector") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); - if(ivalue < 0 || ivalue >660*512) return 0; - popt->sbsector=ivalue; + if(ivalue < 0 || ivalue >660*512) + printk("Invalid or missed parameter:%s=%s,\n",this_char,value); + else popt->sbsector=ivalue; } - else if (!strcmp(this_char,"check") && value) { + if (!strcmp(this_char,"check") && value) { if (value[0] && !value[1] && strchr("rs",*value)) popt->check = *value; else if (!strcmp(value,"relaxed")) popt->check = 'r'; else if (!strcmp(value,"strict")) popt->check = 's'; - else return 0; + else printk("Invalid or missed parameter:%s=%s,\n",this_char,value); } - else if (!strcmp(this_char,"conv") && value) { + if (!strcmp(this_char,"conv") && value) { /* no conversion is done anymore; we still accept the same mount options, but ignore them */ @@ -368,22 +371,24 @@ else if (!strcmp(value,"text")) ; else if (!strcmp(value,"mtext")) ; else if (!strcmp(value,"auto")) ; - else return 0; + else printk("Invalid or missed parameter:%s=%s,\n",this_char,value); } - else if (value && + if (value && (!strcmp(this_char,"block") || !strcmp(this_char,"mode") || !strcmp(this_char,"uid") || !strcmp(this_char,"gid"))) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); - if (*vpnt) return 0; + if (*vpnt) printk("Invalid or missed parameter:%s=%s,\n",this_char,value); + else switch(*this_char) { case 'b': if ( ivalue != 512 && ivalue != 1024 - && ivalue != 2048) return 0; - popt->blocksize = ivalue; + && ivalue != 2048) + printk("Invalid or missed parameter:%s=%s,\n",this_char,value); + else popt->blocksize = ivalue; break; case 'u': popt->uid = ivalue; @@ -396,7 +401,6 @@ break; } } - else return 1; } return 1; } =================================================================================== ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kernel 2.4.27-10: isofs driver ignore some parameters with mount 2005-08-12 8:29 ` kernel 2.4.27-10: isofs driver ignore some parameters with mount Horms @ 2005-08-12 8:41 ` Horms 2005-08-16 1:11 ` Marcelo Tosatti 1 sibling, 0 replies; 6+ messages in thread From: Horms @ 2005-08-12 8:41 UTC (permalink / raw) To: Alexander Pytlev, Marcelo Tosatti, linux-kernel, debian-kernel On Fri, Aug 12, 2005 at 05:29:36PM +0900, Horms wrote: > On Fri, Aug 12, 2005 at 10:44:17AM +0300, Alexander Pytlev wrote: > > Hello Debian, > > > > Kernel 2.4.27-10 > > With mount isofs filesystem, any mount parameters after > > iocharset=,map=,session= are ignored. > > > > Sample: > > > > mount -t isofs -o uid=100,iocharset=koi8-r,gid=100 /dev/cdrom /media/cdrom > > > > gid=100 - was ignored > > > > I look in source and find that problem. I make two patch, simply and full > > (what addeded some functionality - ignore wrong mount parameters) > > Thanks, > > I will try and get the simple version of this patch into the next > Sarge update. > > I have also CCed Marcelo and the LKML for their consideration, > as this problem still seems to be present in the lastest 2.4 tree. > > -- > Horms Marcelo and LKML, here is a rediff of the simple version of the patch from Alexander Pytlev that I forwarded previously. The whitespace in his version had been munged. I haven't tested it, but it looks like it should resolve the problem Alexander reported that mount parameters after iocharset, map and session are ignored. This should apply against current 2.4 git. I took a peek into 2.6, and the code there has seems to have been completely restructured. Signed-off-by: Horms <horms@verge.net.au> --- fs/isofs/inode.c.orig 2005-08-12 17:33:31.000000000 +0900 +++ fs/isofs/inode.c 2005-08-12 17:33:38.000000000 +0900 @@ -340,13 +340,13 @@ else if (!strcmp(value,"acorn")) popt->map = 'a'; else return 0; } - if (!strcmp(this_char,"session") && value) { + else if (!strcmp(this_char,"session") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >99) return 0; popt->session=ivalue+1; } - if (!strcmp(this_char,"sbsector") && value) { + else if (!strcmp(this_char,"sbsector") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >660*512) return 0; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kernel 2.4.27-10: isofs driver ignore some parameters with mount 2005-08-12 8:29 ` kernel 2.4.27-10: isofs driver ignore some parameters with mount Horms 2005-08-12 8:41 ` Horms @ 2005-08-16 1:11 ` Marcelo Tosatti 2005-08-16 5:31 ` Horms 1 sibling, 1 reply; 6+ messages in thread From: Marcelo Tosatti @ 2005-08-16 1:11 UTC (permalink / raw) To: Alexander Pytlev, linux-kernel, debian-kernel Cc: Andrey J. Melnikoff (TEMHOTA), Willy Tarreau Hi folks, On Fri, Aug 12, 2005 at 05:29:36PM +0900, Horms wrote: > On Fri, Aug 12, 2005 at 10:44:17AM +0300, Alexander Pytlev wrote: > > Hello Debian, > > > > Kernel 2.4.27-10 > > With mount isofs filesystem, any mount parameters after > > iocharset=,map=,session= are ignored. > > > > Sample: > > > > mount -t isofs -o uid=100,iocharset=koi8-r,gid=100 /dev/cdrom /media/cdrom > > > > gid=100 - was ignored > > > > I look in source and find that problem. I make two patch, simply and full > > (what addeded some functionality - ignore wrong mount parameters) > > Thanks, > > I will try and get the simple version of this patch into the next > Sarge update. > > I have also CCed Marcelo and the LKML for their consideration, > as this problem still seems to be present in the lastest 2.4 tree. > > -- > Horms > > simply patch: > =================================================================================== > --- kernel-source-2.4.27/fs/isofs/inode.c 2005-05-19 13:29:39.000000000 +0300 > +++ kernel-source/fs/isofs/inode.c 2005-08-11 11:55:12.000000000 +0300 > @@ -340,13 +340,13 @@ > else if (!strcmp(value,"acorn")) popt->map = 'a'; > else return 0; > } > - if (!strcmp(this_char,"session") && value) { > + else if (!strcmp(this_char,"session") && value) { > char * vpnt = value; > unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); > if(ivalue < 0 || ivalue >99) return 0; > popt->session=ivalue+1; > } > - if (!strcmp(this_char,"sbsector") && value) { > + else if (!strcmp(this_char,"sbsector") && value) { > char * vpnt = value; > unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); > if(ivalue < 0 || ivalue >660*512) return 0; > =================================================================================== Neither "sbsector" or "session" parameters are part of the options string used in Alexander's example, so how come this patch can make any difference? Usage of "sbsector" or "session" parameters could explain the above patch making a difference because the buggy, always true "(unsigned long) ivalue < 0" comparison invokes "return 0", but that is not the case. The code after the "popt->iocharset = value;" does not make any sense. It seems that the "*value = 0" assignment can screw up the rest of the string, isnt that the real issue? #ifdef CONFIG_JOLIET if (!strcmp(this_char,"iocharset") && value) { popt->iocharset = value; while (*value && *value != ',') value++; if (value == popt->iocharset) return 0; *value = 0; } else #endif ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kernel 2.4.27-10: isofs driver ignore some parameters with mount 2005-08-16 1:11 ` Marcelo Tosatti @ 2005-08-16 5:31 ` Horms 2005-08-16 8:38 ` Horms 0 siblings, 1 reply; 6+ messages in thread From: Horms @ 2005-08-16 5:31 UTC (permalink / raw) To: Marcelo Tosatti Cc: Alexander Pytlev, linux-kernel, debian-kernel, Andrey J. Melnikoff (TEMHOTA), Willy Tarreau On Mon, Aug 15, 2005 at 10:11:21PM -0300, Marcelo Tosatti wrote: > > Hi folks, > > On Fri, Aug 12, 2005 at 05:29:36PM +0900, Horms wrote: > > On Fri, Aug 12, 2005 at 10:44:17AM +0300, Alexander Pytlev wrote: > > > Hello Debian, > > > > > > Kernel 2.4.27-10 > > > With mount isofs filesystem, any mount parameters after > > > iocharset=,map=,session= are ignored. > > > > > > Sample: > > > > > > mount -t isofs -o uid=100,iocharset=koi8-r,gid=100 /dev/cdrom /media/cdrom > > > > > > gid=100 - was ignored > > > > > > I look in source and find that problem. I make two patch, simply and full > > > (what addeded some functionality - ignore wrong mount parameters) > > > > Thanks, > > > > I will try and get the simple version of this patch into the next > > Sarge update. > > > > I have also CCed Marcelo and the LKML for their consideration, > > as this problem still seems to be present in the lastest 2.4 tree. > > > > -- > > Horms > > > > simply patch: > > =================================================================================== > > --- kernel-source-2.4.27/fs/isofs/inode.c 2005-05-19 13:29:39.000000000 +0300 > > +++ kernel-source/fs/isofs/inode.c 2005-08-11 11:55:12.000000000 +0300 > > @@ -340,13 +340,13 @@ > > else if (!strcmp(value,"acorn")) popt->map = 'a'; > > else return 0; > > } > > - if (!strcmp(this_char,"session") && value) { > > + else if (!strcmp(this_char,"session") && value) { > > char * vpnt = value; > > unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); > > if(ivalue < 0 || ivalue >99) return 0; > > popt->session=ivalue+1; > > } > > - if (!strcmp(this_char,"sbsector") && value) { > > + else if (!strcmp(this_char,"sbsector") && value) { > > char * vpnt = value; > > unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); > > if(ivalue < 0 || ivalue >660*512) return 0; > > =================================================================================== > > Neither "sbsector" or "session" parameters are part of the options string used > in Alexander's example, so how come this patch can make any difference? > > Usage of "sbsector" or "session" parameters could explain the above patch > making a difference because the buggy, always true "(unsigned long) ivalue < 0" > comparison invokes "return 0", but that is not the case. > > The code after the "popt->iocharset = value;" does not make any sense. > > It seems that the "*value = 0" assignment can screw up the rest of the > string, isnt that the real issue? > > #ifdef CONFIG_JOLIET > if (!strcmp(this_char,"iocharset") && value) { > popt->iocharset = value; > while (*value && *value != ',') > value++; > if (value == popt->iocharset) > return 0; > *value = 0; > } else > #endif Sorry about that, while the patch above does seem to be a valid clean up, on further examination I agree that it does not address the problem at hand, and that the problem seems to lie in the *value assignment as you suggest. I wonder if advancing this_char to the character aftter value, if non-NULL would resolve this problem. I'll do some testing, but in the mean time, here is what I have in mind: --- a/fs/isofs/inode.c 2005-08-16 14:22:27.000000000 +0900 +++ b/fs/isofs/inode.c 2005-08-16 14:27:55.000000000 +0900 @@ -329,7 +329,10 @@ value++; if (value == popt->iocharset) return 0; - *value = 0; + if (*value) { + this_char = value + 1; + *value = 0; + } } else #endif if (!strcmp(this_char,"map") && value) { ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kernel 2.4.27-10: isofs driver ignore some parameters with mount 2005-08-16 5:31 ` Horms @ 2005-08-16 8:38 ` Horms 2005-08-16 8:46 ` [PATCH] Bogus code in parsing of iocharset in isofs Horms 0 siblings, 1 reply; 6+ messages in thread From: Horms @ 2005-08-16 8:38 UTC (permalink / raw) To: Marcelo Tosatti, Alexander Pytlev, linux-kernel, debian-kernel, Andrey J. Melnikoff (TEMHOTA), Willy Tarreau Hi, On Marcelo's request I have taken a closer look at this. It seems that Alexander Pytlev's original (simple) patch was correct. Without it the logic looks a bit like this. while (...) { if iocharset ... else if map ... if session ... if sbsector ... else if check ... ... else return 1; } Now, if iocharset, map or session are matched, then none of the if or else if clauses under sbsector will match (that is none of these clauses match iocharset, map or session), and thus the else clause will be hit, and the function will return 1 without parsing any furhter options. With Alexander's fix, the if session and if sbsector clauses become else if, and its easy to see that the return 1 won't be premeturely called. I have tested that this patch works using the testcase options iocharset=koi8-r,gid=100, and checking that gid is set correctly with the patch, and incorrectly without. Here is the patch and signoff again, just for the record. I will send a second patch to clean up the *value = 0 code that Marcelo cast concerns over - its bogus but harmless. Signed-off-by: Horms <horms@verge.net.au> --- a/fs/isofs/inode.c 2005-08-03 14:46:33.000000000 +0900 +++ b/fs/isofs/inode.c 2005-08-16 17:23:04.000000000 +0900 @@ -340,13 +337,13 @@ else if (!strcmp(value,"acorn")) popt->map = 'a'; else return 0; } - if (!strcmp(this_char,"session") && value) { + else if (!strcmp(this_char,"session") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >99) return 0; popt->session=ivalue+1; } - if (!strcmp(this_char,"sbsector") && value) { + else if (!strcmp(this_char,"sbsector") && value) { char * vpnt = value; unsigned int ivalue = simple_strtoul(vpnt, &vpnt, 0); if(ivalue < 0 || ivalue >660*512) return 0; ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Bogus code in parsing of iocharset in isofs 2005-08-16 8:38 ` Horms @ 2005-08-16 8:46 ` Horms 0 siblings, 0 replies; 6+ messages in thread From: Horms @ 2005-08-16 8:46 UTC (permalink / raw) To: Marcelo Tosatti, Alexander Pytlev, linux-kernel, debian-kernel, Andrey J. Melnikoff (TEMHOTA), Willy Tarreau Hi, this is a followup for the patch I sent earlier (like about 2 minutes ago) regarding isofs options parsing. In the course of debuging this Marcelo pointed out the following code #ifdef CONFIG_JOLIET if (!strcmp(this_char,"iocharset") && value) { popt->iocharset = value; while (*value && *value != ',') value++; if (value == popt->iocharset) return 0; *value = 0; } else #endif On inspection it turns out that because of use of strtok(), *value is already NULL terminated, and thus the code snippet above is largely bogus. The following patch should remove the bogus code without changing functionality. Signed-off-by: Horms <horms@verge.net.au> --- ../build-386/fs/isofs/inode.c 2005-08-03 14:46:33.000000000 +0900 +++ fs/isofs/inode.c 2005-08-16 17:23:04.000000000 +0900 @@ -324,12 +324,9 @@ #ifdef CONFIG_JOLIET if (!strcmp(this_char,"iocharset") && value) { - popt->iocharset = value; - while (*value && *value != ',') - value++; - if (value == popt->iocharset) + if (!value) return 0; - *value = 0; + popt->iocharset = value; } else #endif if (!strcmp(this_char,"map") && value) { ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2005-08-16 8:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1853917171.20050812104417@tut.by>
2005-08-12 8:29 ` kernel 2.4.27-10: isofs driver ignore some parameters with mount Horms
2005-08-12 8:41 ` Horms
2005-08-16 1:11 ` Marcelo Tosatti
2005-08-16 5:31 ` Horms
2005-08-16 8:38 ` Horms
2005-08-16 8:46 ` [PATCH] Bogus code in parsing of iocharset in isofs Horms
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox