| Message ID | 20180222192806.67911-1-kolga@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey Olga, On 02/22/2018 02:28 PM, Olga Kornievskaia wrote: > It is possible that userland can pass to the kernel mismatching > inputs for the minorversion. like vers=4.1,minorversion=0. Instead > of making the kernel responposible for 'choosing' the minorversion, > make the userland always responsible for not sending a mismatch. I'm thinking this is probably more of mount problem... mount -t nfs4 -o minorversion=0 server:/export /mnt shouldn't this be a v4.0 mount instead of a 4.2 mount? steved. > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > --- > fs/nfs/super.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/fs/nfs/super.c b/fs/nfs/super.c > index 29bacdc..90c0584 100644 > --- a/fs/nfs/super.c > +++ b/fs/nfs/super.c > @@ -1207,7 +1207,7 @@ static int nfs_parse_mount_options(char *raw, > struct nfs_parsed_mount_data *mnt) > { > char *p, *string, *secdata; > - int rc, sloppy = 0, invalid_option = 0; > + int rc, sloppy = 0, invalid_option = 0, minorversion = -1; > unsigned short protofamily = AF_UNSPEC; > unsigned short mountfamily = AF_UNSPEC; > > @@ -1419,6 +1419,7 @@ static int nfs_parse_mount_options(char *raw, > if (option > NFS4_MAX_MINOR_VERSION) > goto out_invalid_value; > mnt->minorversion = option; > + minorversion = option; > break; > > /* > @@ -1655,6 +1656,9 @@ static int nfs_parse_mount_options(char *raw, > } > } > > + if (minorversion >= 0 && minorversion != mnt->minorversion) > + goto out_mountvers_mismatch; > + > return 1; > > out_mountproto_mismatch: > @@ -1685,6 +1689,10 @@ static int nfs_parse_mount_options(char *raw, > free_secdata(secdata); > printk(KERN_INFO "NFS: security options invalid: %d\n", rc); > return 0; > +out_mountvers_mismatch: > + printk(KERN_INFO "NFS: mismatch versions supplied vers=4.%d and " > + "minorversion=%d\n", mnt->minorversion, minorversion); > + return 0; > } > > /* > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 23, 2018 at 11:24 AM, Steve Dickson <SteveD@redhat.com> wrote: > Hey Olga, > > On 02/22/2018 02:28 PM, Olga Kornievskaia wrote: >> It is possible that userland can pass to the kernel mismatching >> inputs for the minorversion. like vers=4.1,minorversion=0. Instead >> of making the kernel responposible for 'choosing' the minorversion, >> make the userland always responsible for not sending a mismatch. > I'm thinking this is probably more of mount problem... Yes the problem is a broken user land sending incorrect arguments but I still think the kernel needs to do sanity checking on arguments to prevent this from happening again. > mount -t nfs4 -o minorversion=0 server:/export /mnt > > shouldn't this be a v4.0 mount instead of a 4.2 mount? Yes I would think this should create a v4.0 mount. > > steved. > >> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com> >> --- >> fs/nfs/super.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/fs/nfs/super.c b/fs/nfs/super.c >> index 29bacdc..90c0584 100644 >> --- a/fs/nfs/super.c >> +++ b/fs/nfs/super.c >> @@ -1207,7 +1207,7 @@ static int nfs_parse_mount_options(char *raw, >> struct nfs_parsed_mount_data *mnt) >> { >> char *p, *string, *secdata; >> - int rc, sloppy = 0, invalid_option = 0; >> + int rc, sloppy = 0, invalid_option = 0, minorversion = -1; >> unsigned short protofamily = AF_UNSPEC; >> unsigned short mountfamily = AF_UNSPEC; >> >> @@ -1419,6 +1419,7 @@ static int nfs_parse_mount_options(char *raw, >> if (option > NFS4_MAX_MINOR_VERSION) >> goto out_invalid_value; >> mnt->minorversion = option; >> + minorversion = option; >> break; >> >> /* >> @@ -1655,6 +1656,9 @@ static int nfs_parse_mount_options(char *raw, >> } >> } >> >> + if (minorversion >= 0 && minorversion != mnt->minorversion) >> + goto out_mountvers_mismatch; >> + >> return 1; >> >> out_mountproto_mismatch: >> @@ -1685,6 +1689,10 @@ static int nfs_parse_mount_options(char *raw, >> free_secdata(secdata); >> printk(KERN_INFO "NFS: security options invalid: %d\n", rc); >> return 0; >> +out_mountvers_mismatch: >> + printk(KERN_INFO "NFS: mismatch versions supplied vers=4.%d and " >> + "minorversion=%d\n", mnt->minorversion, minorversion); >> + return 0; >> } >> >> /* >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/23/2018 12:05 PM, Olga Kornievskaia wrote: > On Fri, Feb 23, 2018 at 11:24 AM, Steve Dickson <SteveD@redhat.com> wrote: >> Hey Olga, >> >> On 02/22/2018 02:28 PM, Olga Kornievskaia wrote: >>> It is possible that userland can pass to the kernel mismatching >>> inputs for the minorversion. like vers=4.1,minorversion=0. Instead >>> of making the kernel responposible for 'choosing' the minorversion, >>> make the userland always responsible for not sending a mismatch. >> I'm thinking this is probably more of mount problem... > > Yes the problem is a broken user land sending incorrect arguments but > I still think the kernel needs to do sanity checking on arguments to > prevent this from happening again. > >> mount -t nfs4 -o minorversion=0 server:/export /mnt >> >> shouldn't this be a v4.0 mount instead of a 4.2 mount? > > Yes I would think this should create a v4.0 mount. Way back when... there was some talk about deprecating minorversion= flag... Since it is broken, maybe this is a good time to do it? Thoughts? steved. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gRnJpLCAyMDE4LTAyLTIzIGF0IDEzOjQ1IC0wNTAwLCBTdGV2ZSBEaWNrc29uIHdyb3RlOg0K PiANCj4gT24gMDIvMjMvMjAxOCAxMjowNSBQTSwgT2xnYSBLb3JuaWV2c2thaWEgd3JvdGU6DQo+ ID4gT24gRnJpLCBGZWIgMjMsIDIwMTggYXQgMTE6MjQgQU0sIFN0ZXZlIERpY2tzb24gPFN0ZXZl REByZWRoYXQuY29tPg0KPiA+IHdyb3RlOg0KPiA+ID4gSGV5IE9sZ2EsDQo+ID4gPiANCj4gPiA+ IE9uIDAyLzIyLzIwMTggMDI6MjggUE0sIE9sZ2EgS29ybmlldnNrYWlhIHdyb3RlOg0KPiA+ID4g PiBJdCBpcyBwb3NzaWJsZSB0aGF0IHVzZXJsYW5kIGNhbiBwYXNzIHRvIHRoZSBrZXJuZWwgbWlz bWF0Y2hpbmcNCj4gPiA+ID4gaW5wdXRzIGZvciB0aGUgbWlub3J2ZXJzaW9uLiBsaWtlIHZlcnM9 NC4xLG1pbm9ydmVyc2lvbj0wLg0KPiA+ID4gPiBJbnN0ZWFkDQo+ID4gPiA+IG9mIG1ha2luZyB0 aGUga2VybmVsIHJlc3BvbnBvc2libGUgZm9yICdjaG9vc2luZycgdGhlDQo+ID4gPiA+IG1pbm9y dmVyc2lvbiwNCj4gPiA+ID4gbWFrZSB0aGUgdXNlcmxhbmQgYWx3YXlzIHJlc3BvbnNpYmxlIGZv ciBub3Qgc2VuZGluZyBhDQo+ID4gPiA+IG1pc21hdGNoLg0KPiA+ID4gDQo+ID4gPiBJJ20gdGhp bmtpbmcgdGhpcyBpcyBwcm9iYWJseSBtb3JlIG9mIG1vdW50IHByb2JsZW0uLi4NCj4gPiANCj4g PiBZZXMgdGhlIHByb2JsZW0gaXMgYSBicm9rZW4gdXNlciBsYW5kIHNlbmRpbmcgaW5jb3JyZWN0 IGFyZ3VtZW50cw0KPiA+IGJ1dA0KPiA+IEkgc3RpbGwgdGhpbmsgdGhlIGtlcm5lbCBuZWVkcyB0 byBkbyBzYW5pdHkgY2hlY2tpbmcgb24gYXJndW1lbnRzDQo+ID4gdG8NCj4gPiBwcmV2ZW50IHRo aXMgZnJvbSBoYXBwZW5pbmcgYWdhaW4uDQo+ID4gDQo+ID4gPiBtb3VudCAtdCBuZnM0IC1vIG1p bm9ydmVyc2lvbj0wIHNlcnZlcjovZXhwb3J0IC9tbnQNCj4gPiA+IA0KPiA+ID4gc2hvdWxkbid0 IHRoaXMgYmUgYSAgdjQuMCBtb3VudCBpbnN0ZWFkIG9mIGEgNC4yIG1vdW50Pw0KPiA+IA0KPiA+ IFllcyBJIHdvdWxkIHRoaW5rIHRoaXMgc2hvdWxkIGNyZWF0ZSBhIHY0LjAgbW91bnQuDQo+IA0K PiBXYXkgYmFjayB3aGVuLi4uIHRoZXJlIHdhcyBzb21lIHRhbGsgYWJvdXQgZGVwcmVjYXRpbmcg DQo+IG1pbm9ydmVyc2lvbj0gZmxhZy4uLiBTaW5jZSBpdCBpcyBicm9rZW4sIG1heWJlIHRoaXMg aXMgDQo+IGEgZ29vZCB0aW1lIHRvIGRvIGl0Pw0KPiANCj4gVGhvdWdodHM/DQoNCkkndmUgbmV2 ZXIgbGlrZWQgdGhlIHNlcGFyYXRlIHY0LW9ubHkgJ21pbm9ydmVyc2lvbicga2V5d29yZCwgd2hp Y2ggaXMNCndoeSBJIGludHJvZHVjZWQgdGhlICd2ZXJzaW9uPTQueCcgZm9ybWF0LiBNeSBwcmVm ZXJlbmNlIGlzIHRoZXJlZm9yZQ0KdGhhdCB3ZSBjb250aW51ZSB0byBkZXByZWNhdGUgdXNlIG9m ICdtaW5vcnZlcnNpb24nIHdpdGggYSB2aWV3IHRvDQpraWxsaW5nIGl0IG9mZiBjb21wbGV0ZWx5 Lg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQ cmltYXJ5RGF0YQ0KdHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 23, 2018 at 2:09 PM, Trond Myklebust <trondmy@primarydata.com> wrote: > On Fri, 2018-02-23 at 13:45 -0500, Steve Dickson wrote: >> >> On 02/23/2018 12:05 PM, Olga Kornievskaia wrote: >> > On Fri, Feb 23, 2018 at 11:24 AM, Steve Dickson <SteveD@redhat.com> >> > wrote: >> > > Hey Olga, >> > > >> > > On 02/22/2018 02:28 PM, Olga Kornievskaia wrote: >> > > > It is possible that userland can pass to the kernel mismatching >> > > > inputs for the minorversion. like vers=4.1,minorversion=0. >> > > > Instead >> > > > of making the kernel responposible for 'choosing' the >> > > > minorversion, >> > > > make the userland always responsible for not sending a >> > > > mismatch. >> > > >> > > I'm thinking this is probably more of mount problem... >> > >> > Yes the problem is a broken user land sending incorrect arguments >> > but >> > I still think the kernel needs to do sanity checking on arguments >> > to >> > prevent this from happening again. >> > >> > > mount -t nfs4 -o minorversion=0 server:/export /mnt >> > > >> > > shouldn't this be a v4.0 mount instead of a 4.2 mount? >> > >> > Yes I would think this should create a v4.0 mount. >> >> Way back when... there was some talk about deprecating >> minorversion= flag... Since it is broken, maybe this is >> a good time to do it? >> >> Thoughts? > > I've never liked the separate v4-only 'minorversion' keyword, which is > why I introduced the 'version=4.x' format. My preference is therefore > that we continue to deprecate use of 'minorversion' with a view to > killing it off completely. It all seems like a good idea except I wonder how widely used and relied on "minorversion" option is. Steve you might have a good idea about it. > > -- > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/23/2018 02:28 PM, Olga Kornievskaia wrote: > On Fri, Feb 23, 2018 at 2:09 PM, Trond Myklebust > <trondmy@primarydata.com> wrote: >> On Fri, 2018-02-23 at 13:45 -0500, Steve Dickson wrote: >>> >>> On 02/23/2018 12:05 PM, Olga Kornievskaia wrote: >>>> On Fri, Feb 23, 2018 at 11:24 AM, Steve Dickson <SteveD@redhat.com> >>>> wrote: >>>>> Hey Olga, >>>>> >>>>> On 02/22/2018 02:28 PM, Olga Kornievskaia wrote: >>>>>> It is possible that userland can pass to the kernel mismatching >>>>>> inputs for the minorversion. like vers=4.1,minorversion=0. >>>>>> Instead >>>>>> of making the kernel responposible for 'choosing' the >>>>>> minorversion, >>>>>> make the userland always responsible for not sending a >>>>>> mismatch. >>>>> >>>>> I'm thinking this is probably more of mount problem... >>>> >>>> Yes the problem is a broken user land sending incorrect arguments >>>> but >>>> I still think the kernel needs to do sanity checking on arguments >>>> to >>>> prevent this from happening again. >>>> >>>>> mount -t nfs4 -o minorversion=0 server:/export /mnt >>>>> >>>>> shouldn't this be a v4.0 mount instead of a 4.2 mount? >>>> >>>> Yes I would think this should create a v4.0 mount. >>> >>> Way back when... there was some talk about deprecating >>> minorversion= flag... Since it is broken, maybe this is >>> a good time to do it? >>> >>> Thoughts? >> >> I've never liked the separate v4-only 'minorversion' keyword, which is >> why I introduced the 'version=4.x' format. My preference is therefore >> that we continue to deprecate use of 'minorversion' with a view to >> killing it off completely. > > It all seems like a good idea except I wonder how widely used and > relied on "minorversion" option is. Steve you might have a good idea > about it. I have no idea... Actually I'm not sure how to deprecate a mount option... I don't think that has every happen before... But I'm sure it will break something! ;-) steved. > >> >> -- >> Trond Myklebust >> Linux NFS client maintainer, PrimaryData >> trond.myklebust@primarydata.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/super.c b/fs/nfs/super.c index 29bacdc..90c0584 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -1207,7 +1207,7 @@ static int nfs_parse_mount_options(char *raw, struct nfs_parsed_mount_data *mnt) { char *p, *string, *secdata; - int rc, sloppy = 0, invalid_option = 0; + int rc, sloppy = 0, invalid_option = 0, minorversion = -1; unsigned short protofamily = AF_UNSPEC; unsigned short mountfamily = AF_UNSPEC; @@ -1419,6 +1419,7 @@ static int nfs_parse_mount_options(char *raw, if (option > NFS4_MAX_MINOR_VERSION) goto out_invalid_value; mnt->minorversion = option; + minorversion = option; break; /* @@ -1655,6 +1656,9 @@ static int nfs_parse_mount_options(char *raw, } } + if (minorversion >= 0 && minorversion != mnt->minorversion) + goto out_mountvers_mismatch; + return 1; out_mountproto_mismatch: @@ -1685,6 +1689,10 @@ static int nfs_parse_mount_options(char *raw, free_secdata(secdata); printk(KERN_INFO "NFS: security options invalid: %d\n", rc); return 0; +out_mountvers_mismatch: + printk(KERN_INFO "NFS: mismatch versions supplied vers=4.%d and " + "minorversion=%d\n", mnt->minorversion, minorversion); + return 0; } /*
It is possible that userland can pass to the kernel mismatching inputs for the minorversion. like vers=4.1,minorversion=0. Instead of making the kernel responposible for 'choosing' the minorversion, make the userland always responsible for not sending a mismatch. Signed-off-by: Olga Kornievskaia <kolga@netapp.com> --- fs/nfs/super.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)