| Submitter | Gaurav |
|---|---|
| Date | 2011-06-01 05:10:27 |
| Message ID | <1306905027-596-1-git-send-email-gaurav@gluster.com> |
| Download | mbox | patch |
| Permalink | /patch/7344/ |
| State | Accepted |
| Headers | show |
Comments
> - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000); > - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF); > + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof (cs->stbuf.ia_atime)); > + memcpy (&cs->stbuf.ia_mtime, > + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime), > + sizeof (cs->stbuf.ia_mtime)); atime is expected to be the 32 most significant bits and mtime the 32 least significant bits. The diff assumes the architecture to be big-endian. Please use bitmasking + shifting and typecasting to fix the overflow. Avati
Anand Avati wrote: >> - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000); >> - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF); >> + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof (cs->stbuf.ia_atime)); >> + memcpy (&cs->stbuf.ia_mtime, >> + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime), >> + sizeof (cs->stbuf.ia_mtime)); > > atime is expected to be the 32 most significant bits and mtime the 32 > least significant bits. The diff assumes the architecture to be > big-endian. Please use bitmasking + shifting and typecasting to fix > the overflow. Thats fine, we're not interested in the order when storing in atime and mtime, cookieverf is going to be an opaque value from the client which we want to store "somewhere", atime and mtime were conveniently available in this fop. The value does not have to be intelligible, only recoverable through atime and mtime when a stat is performed at a later time. Gaurav, if you're not going to CC me, you'll just delay review of these patches. > > Avati > _______________________________________________ > glusterfs mailing list > glusterfs@dev.gluster.com > http://dev.gluster.com/mailman/listinfo/glusterfs
There is no such requirement of storing most significant 32 bits in atime and least significant 32 bits in mtime, at the time of varification in "nfs3svc_create_stat_cbk" they are compared as two 32 bit values instead of single 64 bit value, so it should work irrespective of endianess. Regards, Gaurav ----- Original Message ----- > From: "Anand Avati" <avati@gluster.com> > To: gaurav@gluster.com > Cc: glusterfs@dev.gluster.com, avati@dev.gluster.com > Sent: Wednesday, June 1, 2011 11:07:40 AM > Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in nfs3_create_exclusive. > > - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000); > > - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF); > > + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof > > (cs->stbuf.ia_atime)); > > + memcpy (&cs->stbuf.ia_mtime, > > + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime), > > + sizeof (cs->stbuf.ia_mtime)); > > atime is expected to be the 32 most significant bits and mtime the 32 > least significant bits. The diff assumes the architecture to be > big-endian. Please use bitmasking + shifting and typecasting to fix > the overflow. > > Avati
Do you mean that those two "pieces" of numbers end up being the atime and mtime of the file on disk? Can you throw some light on this? What is cookieverf? Avati On Wed, Jun 1, 2011 at 11:48 AM, Gangalwar <gaurav@gluster.com> wrote: > There is no such requirement of storing most significant 32 bits in atime and least significant 32 bits in mtime, > at the time of varification in "nfs3svc_create_stat_cbk" they are compared as two 32 bit values instead of single 64 bit value, so it should work irrespective > of endianess. > > Regards, > Gaurav > > ----- Original Message ----- >> From: "Anand Avati" <avati@gluster.com> >> To: gaurav@gluster.com >> Cc: glusterfs@dev.gluster.com, avati@dev.gluster.com >> Sent: Wednesday, June 1, 2011 11:07:40 AM >> Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in nfs3_create_exclusive. >> > - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000); >> > - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF); >> > + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof >> > (cs->stbuf.ia_atime)); >> > + memcpy (&cs->stbuf.ia_mtime, >> > + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime), >> > + sizeof (cs->stbuf.ia_mtime)); >> >> atime is expected to be the 32 most significant bits and mtime the 32 >> least significant bits. The diff assumes the architecture to be >> big-endian. Please use bitmasking + shifting and typecasting to fix >> the overflow. >> >> Avati >
Coockieverf is just a 64 bit data, which we need to store in persistent storage in case of create request in exclusive mode. We use this information to identify duplicate create request in case of retransmission from the same client, client is expected to send same coockieverf in case of retransmission. RFC recommends to store this value in atime and mtime, it has nothing to do with actual atime and mtime values, client is expected to do setattr after successful exclusive create, so these values will be overwritten. Regards, Gaurav ----- Original Message ----- > From: "Anand Avati" <avati@gluster.com> > To: "Gangalwar" <gaurav@gluster.com> > Cc: glusterfs@dev.gluster.com, avati@dev.gluster.com > Sent: Wednesday, June 1, 2011 11:50:22 AM > Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in nfs3_create_exclusive. > Do you mean that those two "pieces" of numbers end up being the atime > and mtime of the file on disk? Can you throw some light on this? What > is cookieverf? > > Avati > > On Wed, Jun 1, 2011 at 11:48 AM, Gangalwar <gaurav@gluster.com> wrote: > > There is no such requirement of storing most significant 32 bits in > > atime and least significant 32 bits in mtime, > > at the time of varification in "nfs3svc_create_stat_cbk" they are > > compared as two 32 bit values instead of single 64 bit value, so it > > should work irrespective > > of endianess. > > > > Regards, > > Gaurav > > > > ----- Original Message ----- > >> From: "Anand Avati" <avati@gluster.com> > >> To: gaurav@gluster.com > >> Cc: glusterfs@dev.gluster.com, avati@dev.gluster.com > >> Sent: Wednesday, June 1, 2011 11:07:40 AM > >> Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in > >> nfs3_create_exclusive. > >> > - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000); > >> > - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF); > >> > + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof > >> > (cs->stbuf.ia_atime)); > >> > + memcpy (&cs->stbuf.ia_mtime, > >> > + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime), > >> > + sizeof (cs->stbuf.ia_mtime)); > >> > >> atime is expected to be the 32 most significant bits and mtime the > >> 32 > >> least significant bits. The diff assumes the architecture to be > >> big-endian. Please use bitmasking + shifting and typecasting to fix > >> the overflow. > >> > >> Avati > >
Right Shehjar, will keep that in mind. Regards, Gaurav ----- Original Message ----- > From: "Shehjar Tikoo" <shehjart@gluster.com> > To: "Anand Avati" <avati@gluster.com> > Cc: gaurav@gluster.com, glusterfs@dev.gluster.com, avati@dev.gluster.com > Sent: Wednesday, June 1, 2011 11:36:49 AM > Subject: Re: [PATCH BUG:2972] NFS : Handle buffer overflow in nfs3_create_exclusive. > Anand Avati wrote: > >> - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000); > >> - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF); > >> + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof > >> (cs->stbuf.ia_atime)); > >> + memcpy (&cs->stbuf.ia_mtime, > >> + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime), > >> + sizeof (cs->stbuf.ia_mtime)); > > > > atime is expected to be the 32 most significant bits and mtime the > > 32 > > least significant bits. The diff assumes the architecture to be > > big-endian. Please use bitmasking + shifting and typecasting to fix > > the overflow. > > Thats fine, we're not interested in the order when storing in atime > and > mtime, cookieverf is going to be an opaque value from the client which > we > want to store "somewhere", atime and mtime were conveniently available > in > this fop. The value does not have to be intelligible, only recoverable > through atime and mtime when a stat is performed at a later time. > > Gaurav, if you're not going to CC me, you'll just delay review of > these > patches. > > > > > > > Avati > > _______________________________________________ > > glusterfs mailing list > > glusterfs@dev.gluster.com > > http://dev.gluster.com/mailman/listinfo/glusterfs
Patch
diff --git a/xlators/nfs/server/src/nfs3.c b/xlators/nfs/server/src/nfs3.c index 13eb1c0..2fc295e 100644 --- a/xlators/nfs/server/src/nfs3.c +++ b/xlators/nfs/server/src/nfs3.c @@ -2437,8 +2437,10 @@ nfs3_create_exclusive (nfs3_call_state_t *cs) /* Storing verifier as a mtime and atime attribute, to store it * in stable storage */ - cs->stbuf.ia_atime = (cs->cookieverf & 0xFFFFFFFF00000000); - cs->stbuf.ia_mtime = (cs->cookieverf & 0x00000000FFFFFFFF); + memcpy (&cs->stbuf.ia_atime, &cs->cookieverf, sizeof (cs->stbuf.ia_atime)); + memcpy (&cs->stbuf.ia_mtime, + ((char *) &cs->cookieverf) + sizeof (cs->stbuf.ia_atime), + sizeof (cs->stbuf.ia_mtime)); cs->setattr_valid |= GF_SET_ATTR_ATIME; cs->setattr_valid |= GF_SET_ATTR_MTIME; nfs_request_user_init (&nfu, cs->req);