LU-540 misc patch for user identity upcall/downcall

1) l_getidentity forgets to allocate supplementary group info space, but access
   such non-allocated memoey without checking.
2) In structure "identity_downcall_data", "idd_gid" is same as "idd_groups[0]",
   remove such redundant information.
3) More sanity check for lprocfs_wr_identity_info().
4) Drop unnecessary memory allocation to simplify mdt_identity_do_upcall().
5) Add padding field in "identity_downcall_data" to resolve align issues.
6) Use new magic to distinguish old version/format downcall.

Change-Id: Ib0dfb37c39a7cb335e29557b68b885afa726789e
Signed-off-by: nasf <yong.fan@whamcloud.com>
Reviewed-on: http://review.whamcloud.com/1160
Tested-by: Hudson
Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Tested-by: Maloo <whamcloud.maloo@gmail.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>

diff --git a/lustre/include/lustre/lustre_user.h b/lustre/include/lustre/lustre_user.h
index 3a7f330..d6f7e88 100644 (file)
--- a/lustre/include/lustre/lustre_user.h
+++ b/lustre/include/lustre/lustre_user.h
@@ -374,7 +374,7 @@ struct if_quotacheck {
         struct obd_uuid         obd_uuid;
 };
 
         struct obd_uuid         obd_uuid;
 };
 

-#define IDENTITY_DOWNCALL_MAGIC 0x6d6dd620
+#define IDENTITY_DOWNCALL_MAGIC 0x6d6dd629

 
 /* permission */
 #define N_PERMS_MAX      64
 
 /* permission */
 #define N_PERMS_MAX      64


@@ -382,6 +382,7 @@ struct if_quotacheck {
 struct perm_downcall_data {
         __u64 pdd_nid;
         __u32 pdd_perm;
 struct perm_downcall_data {
         __u64 pdd_nid;
         __u32 pdd_perm;

+        __u32 pdd_padding;

 };
 
 struct identity_downcall_data {
 };
 
 struct identity_downcall_data {


@@ -390,8 +391,8 @@ struct identity_downcall_data {
         __u32                            idd_uid;
         __u32                            idd_gid;
         __u32                            idd_nperms;
         __u32                            idd_uid;
         __u32                            idd_gid;
         __u32                            idd_nperms;

-        struct perm_downcall_data idd_perms[N_PERMS_MAX];

         __u32                            idd_ngroups;
         __u32                            idd_ngroups;

+        struct perm_downcall_data idd_perms[N_PERMS_MAX];

         __u32                            idd_groups[0];
 };
 
         __u32                            idd_groups[0];
 };
 

diff --git a/lustre/mdt/mdt_identity.c b/lustre/mdt/mdt_identity.c
index f731044..6590e4e 100644 (file)
--- a/lustre/mdt/mdt_identity.c
+++ b/lustre/mdt/mdt_identity.c
@@ -102,7 +102,7 @@ static void mdt_identity_entry_free(struct upcall_cache *cache,
 static int mdt_identity_do_upcall(struct upcall_cache *cache,
                                   struct upcall_cache_entry *entry)
 {
 static int mdt_identity_do_upcall(struct upcall_cache *cache,
                                   struct upcall_cache_entry *entry)
 {

-        char *upcall, keystr[16];
+        char keystr[16];

         char *argv[] = {
                   [0] = cache->uc_upcall,
                   [1] = cache->uc_name,
         char *argv[] = {
                   [0] = cache->uc_upcall,
                   [1] = cache->uc_name,


@@ -115,32 +115,23 @@ static int mdt_identity_do_upcall(struct upcall_cache *cache,
                   [2] = NULL
         };
         struct timeval start, end;
                   [2] = NULL
         };
         struct timeval start, end;

-        int size, rc;
+        int rc;

         ENTRY;
 
         /* There is race condition:
          * "uc_upcall" was changed just after "is_identity_get_disabled" check.
          */
         ENTRY;
 
         /* There is race condition:
          * "uc_upcall" was changed just after "is_identity_get_disabled" check.
          */

-        size = strlen(cache->uc_upcall) + 1;
-        OBD_ALLOC(upcall, size);
-        if (unlikely(!upcall))
-                RETURN(-ENOMEM);
-

         cfs_read_lock(&cache->uc_upcall_rwlock);
         cfs_read_lock(&cache->uc_upcall_rwlock);

-        memcpy(upcall, cache->uc_upcall, size - 1);
-        cfs_read_unlock(&cache->uc_upcall_rwlock);
-        upcall[size - 1] = 0;
-        if (unlikely(!strcmp(upcall, "NONE"))) {
+        CDEBUG(D_INFO, "The upcall is: '%s'\n", cache->uc_upcall);
+
+        if (unlikely(!strcmp(cache->uc_upcall, "NONE"))) {

                 CERROR("no upcall set\n");
                 GOTO(out, rc = -EREMCHG);
         }
 
                 CERROR("no upcall set\n");
                 GOTO(out, rc = -EREMCHG);
         }
 

-        argv[0] = upcall;
-
+        argv[0] = cache->uc_upcall;

         snprintf(keystr, sizeof(keystr), LPU64, entry->ue_key);
 
         snprintf(keystr, sizeof(keystr), LPU64, entry->ue_key);
 

-        CDEBUG(D_INFO, "The upcall is: '%s'\n", cache->uc_upcall);
-

         cfs_gettimeofday(&start);
         rc = USERMODEHELPER(argv[0], argv, envp);
         cfs_gettimeofday(&end);
         cfs_gettimeofday(&start);
         rc = USERMODEHELPER(argv[0], argv, envp);
         cfs_gettimeofday(&end);


@@ -158,7 +149,7 @@ static int mdt_identity_do_upcall(struct upcall_cache *cache,
         }
         EXIT;
 out:
         }
         EXIT;
 out:

-        OBD_FREE(upcall, size);
+        cfs_read_unlock(&cache->uc_upcall_rwlock);

         return rc;
 }
 
         return rc;
 }
 


@@ -168,7 +159,7 @@ static int mdt_identity_parse_downcall(struct upcall_cache *cache,
 {
         struct md_identity *identity = &entry->u.identity;
         struct identity_downcall_data *data = args;
 {
         struct md_identity *identity = &entry->u.identity;
         struct identity_downcall_data *data = args;

-        cfs_group_info_t *ginfo;
+        cfs_group_info_t *ginfo = NULL;

         struct md_perm *perms = NULL;
         int size, i;
         ENTRY;
         struct md_perm *perms = NULL;
         int size, i;
         ENTRY;


@@ -177,14 +168,16 @@ static int mdt_identity_parse_downcall(struct upcall_cache *cache,
         if (data->idd_ngroups > NGROUPS_MAX)
                 RETURN(-E2BIG);
 
         if (data->idd_ngroups > NGROUPS_MAX)
                 RETURN(-E2BIG);
 

-        ginfo = cfs_groups_alloc(data->idd_ngroups);
-        if (!ginfo) {
-                CERROR("failed to alloc %d groups\n", data->idd_ngroups);
-                RETURN(-ENOMEM);
-        }
+        if (data->idd_ngroups > 0) {
+                ginfo = cfs_groups_alloc(data->idd_ngroups);
+                if (!ginfo) {
+                        CERROR("failed to alloc %d groups\n", data->idd_ngroups);
+                        RETURN(-ENOMEM);
+                }

 
 

-        lustre_groups_from_list(ginfo, data->idd_groups);
-        lustre_groups_sort(ginfo);
+                lustre_groups_from_list(ginfo, data->idd_groups);
+                lustre_groups_sort(ginfo);
+        }

 
         if (data->idd_nperms) {
                 size = data->idd_nperms * sizeof(*perms);
 
         if (data->idd_nperms) {
                 size = data->idd_nperms * sizeof(*perms);


@@ -192,7 +185,8 @@ static int mdt_identity_parse_downcall(struct upcall_cache *cache,
                 if (!perms) {
                         CERROR("failed to alloc %d permissions\n",
                                data->idd_nperms);
                 if (!perms) {
                         CERROR("failed to alloc %d permissions\n",
                                data->idd_nperms);

-                        cfs_put_group_info(ginfo);
+                        if (ginfo != NULL)
+                                cfs_put_group_info(ginfo);

                         RETURN(-ENOMEM);
                 }
 
                         RETURN(-ENOMEM);
                 }
 

diff --git a/lustre/mdt/mdt_lproc.c b/lustre/mdt/mdt_lproc.c
index 47f75cd..a95bbff 100644 (file)
--- a/lustre/mdt/mdt_lproc.c
+++ b/lustre/mdt/mdt_lproc.c
@@ -281,52 +281,51 @@ static int lprocfs_wr_identity_info(struct file *file, const char *buffer,
 {
         struct obd_device *obd = data;
         struct mdt_device *mdt = mdt_dev(obd->obd_lu_dev);
 {
         struct obd_device *obd = data;
         struct mdt_device *mdt = mdt_dev(obd->obd_lu_dev);

-        struct identity_downcall_data sparam, *param = &sparam;
-        int size = 0, rc = 0;
+        struct identity_downcall_data *param;
+        int size = sizeof(*param), rc, checked = 0;

 
 

-        if (count < sizeof(*param)) {
-                CERROR("%s: invalid data size %lu\n", obd->obd_name, count);
-                return count;
+again:
+        if (count < size) {
+                CERROR("%s: invalid data count = %lu, size = %d\n",
+                       obd->obd_name, count, size);
+                return -EINVAL;

         }
 
         }
 

-        if (cfs_copy_from_user(&sparam, buffer, sizeof(sparam))) {
+        OBD_ALLOC(param, size);
+        if (param == NULL)
+                return -ENOMEM;
+
+        if (cfs_copy_from_user(param, buffer, size)) {

                 CERROR("%s: bad identity data\n", obd->obd_name);
                 GOTO(out, rc = -EFAULT);
         }
 
                 CERROR("%s: bad identity data\n", obd->obd_name);
                 GOTO(out, rc = -EFAULT);
         }
 

-        if (sparam.idd_magic != IDENTITY_DOWNCALL_MAGIC) {
-                CERROR("%s: MDS identity downcall bad params\n", obd->obd_name);
-                GOTO(out, rc = -EINVAL);
-        }
+        if (checked == 0) {
+                checked = 1;
+                if (param->idd_magic != IDENTITY_DOWNCALL_MAGIC) {
+                        CERROR("%s: MDS identity downcall bad params\n",
+                               obd->obd_name);
+                        GOTO(out, rc = -EINVAL);
+                }

 
 

-        if (sparam.idd_nperms > N_PERMS_MAX) {
-                CERROR("%s: perm count %d more than maximum %d\n",
-                       obd->obd_name, sparam.idd_nperms, N_PERMS_MAX);
-                GOTO(out, rc = -EINVAL);
-        }
+                if (param->idd_nperms > N_PERMS_MAX) {
+                        CERROR("%s: perm count %d more than maximum %d\n",
+                               obd->obd_name, param->idd_nperms, N_PERMS_MAX);
+                        GOTO(out, rc = -EINVAL);
+                }

 
 

-        if (sparam.idd_ngroups > NGROUPS_MAX) {
-                CERROR("%s: group count %d more than maximum %d\n",
-                       obd->obd_name, sparam.idd_ngroups, NGROUPS_MAX);
-                GOTO(out, rc = -EINVAL);
-        }
+                if (param->idd_ngroups > NGROUPS_MAX) {
+                        CERROR("%s: group count %d more than maximum %d\n",
+                               obd->obd_name, param->idd_ngroups, NGROUPS_MAX);
+                        GOTO(out, rc = -EINVAL);
+                }

 
 

-        if (sparam.idd_ngroups) {
-                size = offsetof(struct identity_downcall_data,
-                                idd_groups[sparam.idd_ngroups]);
-                OBD_ALLOC(param, size);
-                if (!param) {
-                        CERROR("%s: fail to alloc %d bytes for uid %u"
-                               " with %d groups\n", obd->obd_name, size,
-                               sparam.idd_uid, sparam.idd_ngroups);
-                        param = &sparam;
-                        param->idd_ngroups = 0;
-                } else if (cfs_copy_from_user(param, buffer, size)) {
-                        CERROR("%s: uid %u bad supplementary group data\n",
-                               obd->obd_name, sparam.idd_uid);
+                if (param->idd_ngroups) {
+                        rc = param->idd_ngroups; /* save idd_ngroups */

                         OBD_FREE(param, size);
                         OBD_FREE(param, size);

-                        param = &sparam;
-                        param->idd_ngroups = 0;
+                        size = offsetof(struct identity_downcall_data,
+                                        idd_groups[rc]);
+                        goto again;

                 }
         }
 
                 }
         }
 


@@ -334,10 +333,10 @@ static int lprocfs_wr_identity_info(struct file *file, const char *buffer,
                                    param->idd_uid, param);
 
 out:
                                    param->idd_uid, param);
 
 out:

-        if (param && (param != &sparam))
+        if (param != NULL)

                 OBD_FREE(param, size);
 
                 OBD_FREE(param, size);
 

-        return rc ?: count;
+        return rc ? rc : count;

 }
 
 /* for debug only */
 }
 
 /* for debug only */

diff --git a/lustre/utils/l_getidentity.c b/lustre/utils/l_getidentity.c
index ee4b645..7e66979 100644 (file)
--- a/lustre/utils/l_getidentity.c
+++ b/lustre/utils/l_getidentity.c
@@ -76,7 +76,7 @@ static void usage(void)
         fprintf(stderr,
                 "\nusage: %s {mdtname} {uid}\n"
                 "Normally invoked as an upcall from Lustre, set via:\n"
         fprintf(stderr,
                 "\nusage: %s {mdtname} {uid}\n"
                 "Normally invoked as an upcall from Lustre, set via:\n"

-                "  /proc/fs/lustre/mdt/{mdtname}/identity_upcall\n",
+                "/proc/fs/lustre/mdt/${mdtname}/identity_upcall\n",

                 progname);
 }
 
                 progname);
 }
 


@@ -98,9 +98,9 @@ static void errlog(const char *fmt, ...)
         closelog();
 }
 
         closelog();
 }
 

-int get_groups_local(struct identity_downcall_data *data)
+int get_groups_local(struct identity_downcall_data *data,
+                     unsigned int maxgroups)

 {
 {

-        int maxgroups;

         gid_t *groups;
         unsigned int ngroups = 0;
         struct passwd *pw;
         gid_t *groups;
         unsigned int ngroups = 0;
         struct passwd *pw;


@@ -115,27 +115,24 @@ int get_groups_local(struct identity_downcall_data *data)
                 data->idd_err = errno ? errno : EIDRM;
                 return -1;
         }
                 data->idd_err = errno ? errno : EIDRM;
                 return -1;
         }

-        data->idd_gid = pw->pw_gid;

 
 

+        data->idd_gid = pw->pw_gid;

         namelen = sysconf(_SC_LOGIN_NAME_MAX);
         if (namelen < _POSIX_LOGIN_NAME_MAX)
                 namelen = _POSIX_LOGIN_NAME_MAX;
         namelen = sysconf(_SC_LOGIN_NAME_MAX);
         if (namelen < _POSIX_LOGIN_NAME_MAX)
                 namelen = _POSIX_LOGIN_NAME_MAX;

+

         pw_name = (char *)malloc(namelen);
         if (!pw_name) {
                 errlog("malloc error\n");
                 data->idd_err = errno;
                 return -1;
         }
         pw_name = (char *)malloc(namelen);
         if (!pw_name) {
                 errlog("malloc error\n");
                 data->idd_err = errno;
                 return -1;
         }

+

         memset(pw_name, 0, namelen);
         strncpy(pw_name, pw->pw_name, namelen - 1);
         memset(pw_name, 0, namelen);
         strncpy(pw_name, pw->pw_name, namelen - 1);

-
-        maxgroups = sysconf(_SC_NGROUPS_MAX);
-        if (maxgroups > NGROUPS_MAX)
-                maxgroups = NGROUPS_MAX;

         groups = data->idd_groups;
 
         groups = data->idd_groups;
 

-        groups[ngroups++] = pw->pw_gid;
-        while ((gr = getgrent())) {
+        while ((gr = getgrent()) && ngroups < maxgroups) {

                 if (gr->gr_gid == groups[0])
                         continue;
                 if (!gr->gr_mem)
                 if (gr->gr_gid == groups[0])
                         continue;
                 if (!gr->gr_mem)


@@ -146,11 +143,10 @@ int get_groups_local(struct identity_downcall_data *data)
                                 break;
                         }
                 }
                                 break;
                         }
                 }

-                if (ngroups == maxgroups)
-                        break;

         }
         endgrent();
         }
         endgrent();

-        qsort(groups, ngroups, sizeof(*groups), compare_u32);
+        if (ngroups > 0)
+                qsort(groups, ngroups, sizeof(*groups), compare_u32);

         data->idd_ngroups = ngroups;
 
         free(pw_name);
         data->idd_ngroups = ngroups;
 
         free(pw_name);


@@ -179,7 +175,6 @@ static inline int match_uid(uid_t uid, const char *str)
         uid2 = strtoul(str, &end, 0);
         if (*end)
                 return 0;
         uid2 = strtoul(str, &end, 0);
         if (*end)
                 return 0;

-

         return (uid == uid2);
 }
 
         return (uid == uid2);
 }
 


@@ -340,20 +335,36 @@ int parse_perm_line(struct identity_downcall_data *data, char *line)
         return 0;
 }
 
         return 0;
 }
 

-int get_perms(FILE *fp, struct identity_downcall_data *data)
+int get_perms(struct identity_downcall_data *data)

 {
 {

+        FILE *fp;

         char line[1024];
 
         char line[1024];
 

+        fp = fopen(PERM_PATHNAME, "r");
+        if (fp == NULL) {
+                if (errno == ENOENT) {
+                        return 0;
+                } else {
+                        errlog("open %s failed: %s\n",
+                               PERM_PATHNAME, strerror(errno));
+                        data->idd_err = errno;
+                        return -1;
+                }
+        }
+

         while (fgets(line, 1024, fp)) {
                 if (comment_line(line))
                         continue;
 
                 if (parse_perm_line(data, line)) {
                         errlog("parse line %s failed!\n", line);
         while (fgets(line, 1024, fp)) {
                 if (comment_line(line))
                         continue;
 
                 if (parse_perm_line(data, line)) {
                         errlog("parse line %s failed!\n", line);

+                        data->idd_err = EINVAL;
+                        fclose(fp);

                         return -1;
                 }
         }
 
                         return -1;
                 }
         }
 

+        fclose(fp);

         return 0;
 }
 
         return 0;
 }
 


@@ -367,9 +378,9 @@ static void show_result(struct identity_downcall_data *data)
                 return;
         }
 
                 return;
         }
 

-        printf("uid=%d gid=", data->idd_uid);
+        printf("uid=%d gid=%d", data->idd_uid, data->idd_gid);

         for (i = 0; i < data->idd_ngroups; i++)
         for (i = 0; i < data->idd_ngroups; i++)

-                printf("%s%u", i > 0 ? "," : "", data->idd_groups[i]);
+                printf(",%u", data->idd_groups[i]);

         printf("\n");
         printf("permissions:\n"
                "  nid\t\t\tperm\n");
         printf("\n");
         printf("permissions:\n"
                "  nid\t\t\tperm\n");


@@ -385,56 +396,54 @@ static void show_result(struct identity_downcall_data *data)
 
 int main(int argc, char **argv)
 {
 
 int main(int argc, char **argv)
 {

-        FILE *perms_fp;

         char *end;
         char *end;

-        struct identity_downcall_data *data;
+        struct identity_downcall_data *data = NULL;

         char procname[1024];
         unsigned long uid;
         char procname[1024];
         unsigned long uid;

-        int fd, rc;
+        int fd, rc = -EINVAL, size, maxgroups;

 
         progname = basename(argv[0]);
 
         progname = basename(argv[0]);

-

         if (argc != 3) {
                 usage();
         if (argc != 3) {
                 usage();

-                return 1;
+                goto out;

         }
 
         uid = strtoul(argv[2], &end, 0);
         if (*end) {
                 errlog("%s: invalid uid '%s'\n", progname, argv[2]);
         }
 
         uid = strtoul(argv[2], &end, 0);
         if (*end) {
                 errlog("%s: invalid uid '%s'\n", progname, argv[2]);

-                usage();
-                return 1;
+                goto out;

         }
 
         }
 

-        data = malloc(sizeof(*data));
+        maxgroups = sysconf(_SC_NGROUPS_MAX);
+        if (maxgroups > NGROUPS_MAX)
+                maxgroups = NGROUPS_MAX;
+
+        size = offsetof(struct identity_downcall_data, idd_groups[maxgroups]);
+        data = malloc(size);

         if (!data) {
         if (!data) {

-                errlog("malloc identity downcall data(%d) failed!\n",
-                       sizeof(*data));
-                return 1;
+                errlog("malloc identity downcall data(%d) failed!\n", size);
+                rc = -ENOMEM;
+                goto out;

         }
         }

-        memset(data, 0, sizeof(*data));
+
+        memset(data, 0, size);

         data->idd_magic = IDENTITY_DOWNCALL_MAGIC;
         data->idd_uid = uid;
         data->idd_magic = IDENTITY_DOWNCALL_MAGIC;
         data->idd_uid = uid;

-

         /* get groups for uid */
         /* get groups for uid */

-        rc = get_groups_local(data);
+        rc = get_groups_local(data, maxgroups);

         if (rc)
                 goto downcall;
 
         if (rc)
                 goto downcall;
 

+        size = offsetof(struct identity_downcall_data,
+                        idd_groups[data->idd_ngroups]);

         /* read permission database */
         /* read permission database */

-        perms_fp = fopen(PERM_PATHNAME, "r");
-        if (perms_fp) {
-                get_perms(perms_fp, data);
-                fclose(perms_fp);
-        } else if (errno != ENOENT) {
-                errlog("open %s failed: %s\n",
-                       PERM_PATHNAME, strerror(errno));
-        }
+        rc = get_perms(data);

 
 downcall:
         if (getenv("L_GETIDENTITY_TEST")) {
                 show_result(data);
 
 downcall:
         if (getenv("L_GETIDENTITY_TEST")) {
                 show_result(data);

-                return 0;
+                rc = 0;
+                goto out;

         }
 
         snprintf(procname, sizeof(procname),
         }
 
         snprintf(procname, sizeof(procname),


@@ -442,15 +451,21 @@ downcall:
         fd = open(procname, O_WRONLY);
         if (fd < 0) {
                 errlog("can't open file %s: %s\n", procname, strerror(errno));
         fd = open(procname, O_WRONLY);
         if (fd < 0) {
                 errlog("can't open file %s: %s\n", procname, strerror(errno));

-                return 1;
+                rc = -1;
+                goto out;

         }
 
         }
 

-        rc = write(fd, data, sizeof(*data));
+        rc = write(fd, data, size);

         close(fd);
         close(fd);

-        if (rc != sizeof(*data)) {
+        if (rc != size) {

                 errlog("partial write ret %d: %s\n", rc, strerror(errno));
                 errlog("partial write ret %d: %s\n", rc, strerror(errno));

-                return 1;
+                rc = -1;
+        } else {
+                rc = 0;

         }
 
         }
 

-        return 0;
+out:
+        if (data != NULL)
+                free(data);
+        return rc;

 }
 }