* 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