On Fri, 19 Oct 2012 08:20:47 -0400 Jeff Layton <[email protected]> wrote:
> The cifs.idmap keytype always allocates memory to hold the payload from > userspace. In the common case where we're translating a SID to a UID or > GID, we're allocating memory to hold something that's less than or equal > to the size of a pointer. > > When the payload is the same size as a pointer or smaller, just store > it in the payload.value union member instead. That saves us an extra > allocation on the sid_to_id upcall. > > Note that we have to take extra care to check the datalen when we > go to dereference the .data pointer in the union, but the callers > now check that as a matter of course anyway. > > Signed-off-by: Jeff Layton <[email protected]> > --- > fs/cifs/cifsacl.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c > index f4508ee..12d70ee 100644 > --- a/fs/cifs/cifsacl.c > +++ b/fs/cifs/cifsacl.c > @@ -49,6 +49,20 @@ cifs_idmap_key_instantiate(struct key *key, struct > key_preparsed_payload *prep) > { > char *payload; > > + /* > + * If the payload is less than or equal to the size of a pointer, then > + * an allocation here is wasteful. Just copy the data directly to the > + * payload.value union member instead. > + * > + * With this however, you must check the datalen before trying to > + * dereference payload.data! > + */ > + if (prep->datalen <= sizeof(void *)) { > + key->payload.value = 0; > + memcpy(&key->payload.value, prep->data, prep->datalen); > + key->datalen = prep->datalen; > + return 0; > + } > payload = kmalloc(prep->datalen, GFP_KERNEL); > if (!payload) > return -ENOMEM; > @@ -62,7 +76,8 @@ cifs_idmap_key_instantiate(struct key *key, struct > key_preparsed_payload *prep) > static inline void > cifs_idmap_key_destroy(struct key *key) > { > - kfree(key->payload.data); > + if (key->datalen > sizeof(void *)) > + kfree(key->payload.data); > } > > static struct key_type cifs_idmap_key_type = { > @@ -245,7 +260,7 @@ sid_to_id(struct cifs_sb_info *cifs_sb, struct cifs_sid > *psid, > * probably a safe assumption but might be better to check based on > * sidtype. > */ > - if (sidkey->datalen < sizeof(uid_t)) { > + if (sidkey->datalen != sizeof(uid_t)) { > rc = -EIO; > cFYI(1, "%s: Downcall contained malformed key " > "(datalen=%hu)", __func__, sidkey->datalen); > @@ -253,9 +268,9 @@ sid_to_id(struct cifs_sb_info *cifs_sb, struct cifs_sid > *psid, > } > > if (sidtype == SIDOWNER) > - fuid = *(uid_t *)sidkey->payload.value; > + fuid = (uid_t)sidkey->payload.value; > else > - fgid = *(gid_t *)sidkey->payload.value; > + fgid = (gid_t)sidkey->payload.value; > Finally got a BE machine to test this stuff on, and figured out that the above delta is wrong. We're memcpy'ing the payload (sizeof(uid_t), which is same as unsigned int) into the payload.value (which is an unsigned long) The above block then tries to cast the result of that to a uid_t/gid_t, which works fine on a little-endian machine. Not so much on a BE one... I'll send a respin for this patch once I've done a bit more testing. Thanks, -- Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
