* [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times.
@ 2012-12-26 5:04 Chen Gang
2012-12-27 8:06 ` Chen Gang
2012-12-31 12:06 ` Jan Kara
0 siblings, 2 replies; 7+ messages in thread
From: Chen Gang @ 2012-12-26 5:04 UTC (permalink / raw)
To: Theodore Ts'o, jack, akpm; +Cc: linux-ext4
Hello Theodore Ts'o
in fs/ext3/supper.c
for function set_qf_name:
sbi->s_qf_names[qtype] may already have owned a memory (line 919..925)
we set sbi->s_qf_names[qtype] = qname directly without checking (line 926)
for function clear_qf_name:
we set sbi->s_qf_names[qtype] = NULL (line 942..952)
for function parse_options:
we can call set_qf_name or clear_qf_name with USR or GRP many times.
we find parameters not mind whether they are repeated. (line 975..985)
so we may call set_qf_name or clear_qf_name several times.
also may first call set_qf_name, then call clear_qf_name.
in this situation, we will get memory leak.
please help check this suggestion whether valid (I find it by code review).
regards.
gchen.
901 static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
902 {
903 struct ext3_sb_info *sbi = EXT3_SB(sb);
904 char *qname;
905
906 if (sb_any_quota_loaded(sb) &&
907 !sbi->s_qf_names[qtype]) {
908 ext3_msg(sb, KERN_ERR,
909 "Cannot change journaled "
910 "quota options when quota turned on");
911 return 0;
912 }
913 qname = match_strdup(args);
914 if (!qname) {
915 ext3_msg(sb, KERN_ERR,
916 "Not enough memory for storing quotafile name");
917 return 0;
918 }
919 if (sbi->s_qf_names[qtype] &&
920 strcmp(sbi->s_qf_names[qtype], qname)) {
921 ext3_msg(sb, KERN_ERR,
922 "%s quota file already specified", QTYPE2NAME(qtype));
923 kfree(qname);
924 return 0;
925 }
926 sbi->s_qf_names[qtype] = qname;
927 if (strchr(sbi->s_qf_names[qtype], '/')) {
928 ext3_msg(sb, KERN_ERR,
929 "quotafile must be on filesystem root");
930 kfree(sbi->s_qf_names[qtype]);
931 sbi->s_qf_names[qtype] = NULL;
932 return 0;
933 }
934 set_opt(sbi->s_mount_opt, QUOTA);
935 return 1;
936 }
937
938 static int clear_qf_name(struct super_block *sb, int qtype) {
939
940 struct ext3_sb_info *sbi = EXT3_SB(sb);
941
942 if (sb_any_quota_loaded(sb) &&
943 sbi->s_qf_names[qtype]) {
944 ext3_msg(sb, KERN_ERR, "Cannot change journaled quota options"
945 " when quota turned on");
946 return 0;
947 }
948 /*
949 * The space will be released later when all options are confirmed
950 * to be correct
951 */
952 sbi->s_qf_names[qtype] = NULL;
953 return 1;
954 }
955 #endif
956
957 static int parse_options (char *options, struct super_block *sb,
958 unsigned int *inum, unsigned long *journal_devnum,
959 ext3_fsblk_t *n_blocks_count, int is_remount)
960 {
961 struct ext3_sb_info *sbi = EXT3_SB(sb);
962 char * p;
963 substring_t args[MAX_OPT_ARGS];
964 int data_opt = 0;
965 int option;
966 kuid_t uid;
967 kgid_t gid;
968 #ifdef CONFIG_QUOTA
969 int qfmt;
970 #endif
971
972 if (!options)
973 return 1;
974
975 while ((p = strsep (&options, ",")) != NULL) {
976 int token;
977 if (!*p)
978 continue;
979 /*
980 * Initialize args struct so we know whether arg was
981 * found; some options take optional arguments.
982 */
983 args[0].to = args[0].from = NULL;
984 token = match_token(p, tokens, args);
985 switch (token) {
986 case Opt_bsd_df:
987 clear_opt (sbi->s_mount_opt, MINIX_DF);
988 break;
989 case Opt_minix_df:
990 set_opt (sbi->s_mount_opt, MINIX_DF);
991 break;
992 case Opt_grpid:
993 set_opt (sbi->s_mount_opt, GRPID);
955 #endif
956
957 static int parse_options (char *options, struct super_block *sb,
958 unsigned int *inum, unsigned long *journal_devnum,
959 ext3_fsblk_t *n_blocks_count, int is_remount)
960 {
961 struct ext3_sb_info *sbi = EXT3_SB(sb);
962 char * p;
963 substring_t args[MAX_OPT_ARGS];
964 int data_opt = 0;
965 int option;
966 kuid_t uid;
967 kgid_t gid;
968 #ifdef CONFIG_QUOTA
969 int qfmt;
970 #endif
971
972 if (!options)
973 return 1;
974
975 while ((p = strsep (&options, ",")) != NULL) {
976 int token;
977 if (!*p)
978 continue;
979 /*
980 * Initialize args struct so we know whether arg was
981 * found; some options take optional arguments.
982 */
983 args[0].to = args[0].from = NULL;
984 token = match_token(p, tokens, args);
985 switch (token) {
986 case Opt_bsd_df:
987 clear_opt (sbi->s_mount_opt, MINIX_DF);
988 break;
989 case Opt_minix_df:
990 set_opt (sbi->s_mount_opt, MINIX_DF);
991 break;
992 case Opt_grpid:
993 set_opt (sbi->s_mount_opt, GRPID);
994 break;
995 case Opt_nogrpid:
996 clear_opt (sbi->s_mount_opt, GRPID);
997 break;
998 case Opt_resuid:
999 if (match_int(&args[0], &option))
1000 return 0;
1001 uid = make_kuid(current_user_ns(), option);
1002 if (!uid_valid(uid)) {
1003 ext3_msg(sb, KERN_ERR, "Invalid uid value %d", option);
1004 return 0;
1005
1006 }
1007 sbi->s_resuid = uid;
1008 break;
1009 case Opt_resgid:
1010 if (match_int(&args[0], &option))
1011 return 0;
1012 gid = make_kgid(current_user_ns(), option);
1013 if (!gid_valid(gid)) {
1014 ext3_msg(sb, KERN_ERR, "Invalid gid value %d", option);
1015 return 0;
1016 }
1017 sbi->s_resgid = gid;
1018 break;
1019 case Opt_sb:
1020 /* handled by get_sb_block() instead of here */
1021 /* *sb_block = match_int(&args[0]); */
1022 break;
1023 case Opt_err_panic:
1024 clear_opt (sbi->s_mount_opt, ERRORS_CONT);
1025 clear_opt (sbi->s_mount_opt, ERRORS_RO);
1026 set_opt (sbi->s_mount_opt, ERRORS_PANIC);
1027 break;
1028 case Opt_err_ro:
1029 clear_opt (sbi->s_mount_opt, ERRORS_CONT);
1030 clear_opt (sbi->s_mount_opt, ERRORS_PANIC);
1031 set_opt (sbi->s_mount_opt, ERRORS_RO);
1032 break;
1033 case Opt_err_cont:
1034 clear_opt (sbi->s_mount_opt, ERRORS_RO);
1035 clear_opt (sbi->s_mount_opt, ERRORS_PANIC);
1036 set_opt (sbi->s_mount_opt, ERRORS_CONT);
1037 break;
1038 case Opt_nouid32:
1039 set_opt (sbi->s_mount_opt, NO_UID32);
1040 break;
1041 case Opt_nocheck:
1042 clear_opt (sbi->s_mount_opt, CHECK);
1043 break;
1044 case Opt_debug:
1045 set_opt (sbi->s_mount_opt, DEBUG);
1046 break;
1047 case Opt_oldalloc:
1048 ext3_msg(sb, KERN_WARNING,
1049 "Ignoring deprecated oldalloc option");
1050 break;
1051 case Opt_orlov:
1052 ext3_msg(sb, KERN_WARNING,
1053 "Ignoring deprecated orlov option");
1054 break;
1055 #ifdef CONFIG_EXT3_FS_XATTR
1056 case Opt_user_xattr:
1057 set_opt (sbi->s_mount_opt, XATTR_USER);
1058 break;
1059 case Opt_nouser_xattr:
1060 clear_opt (sbi->s_mount_opt, XATTR_USER);
1061 break;
1062 #else
1063 case Opt_user_xattr:
1064 case Opt_nouser_xattr:
1065 ext3_msg(sb, KERN_INFO,
1066 "(no)user_xattr options not supported");
1067 break;
1068 #endif
1069 #ifdef CONFIG_EXT3_FS_POSIX_ACL
1070 case Opt_acl:
1071 set_opt(sbi->s_mount_opt, POSIX_ACL);
1072 break;
1073 case Opt_noacl:
1074 clear_opt(sbi->s_mount_opt, POSIX_ACL);
1075 break;
1076 #else
1077 case Opt_acl:
1078 case Opt_noacl:
1079 ext3_msg(sb, KERN_INFO,
1080 "(no)acl options not supported");
1081 break;
1082 #endif
1083 case Opt_reservation:
1084 set_opt(sbi->s_mount_opt, RESERVATION);
1085 break;
1086 case Opt_noreservation:
1087 clear_opt(sbi->s_mount_opt, RESERVATION);
1088 break;
1089 case Opt_journal_update:
1090 /* @@@ FIXME */
1091 /* Eventually we will want to be able to create
1092 a journal file here. For now, only allow the
1093 user to specify an existing inode to be the
1094 journal file. */
1095 if (is_remount) {
1096 ext3_msg(sb, KERN_ERR, "error: cannot specify "
1097 "journal on remount");
1098 return 0;
1099 }
1100 set_opt (sbi->s_mount_opt, UPDATE_JOURNAL);
1101 break;
1102 case Opt_journal_inum:
1103 if (is_remount) {
1104 ext3_msg(sb, KERN_ERR, "error: cannot specify "
1105 "journal on remount");
1106 return 0;
1107 }
1108 if (match_int(&args[0], &option))
1109 return 0;
1110 *inum = option;
1111 break;
1112 case Opt_journal_dev:
1113 if (is_remount) {
1114 ext3_msg(sb, KERN_ERR, "error: cannot specify "
1115 "journal on remount");
1116 return 0;
1117 }
1118 if (match_int(&args[0], &option))
1119 return 0;
1120 *journal_devnum = option;
1121 break;
1122 case Opt_noload:
1123 set_opt (sbi->s_mount_opt, NOLOAD);
1124 break;
1125 case Opt_commit:
1126 if (match_int(&args[0], &option))
1127 return 0;
1128 if (option < 0)
1129 return 0;
1130 if (option == 0)
1131 option = JBD_DEFAULT_MAX_COMMIT_AGE;
1132 sbi->s_commit_interval = HZ * option;
1133 break;
1134 case Opt_data_journal:
1135 data_opt = EXT3_MOUNT_JOURNAL_DATA;
1136 goto datacheck;
1137 case Opt_data_ordered:
1138 data_opt = EXT3_MOUNT_ORDERED_DATA;
1139 goto datacheck;
1140 case Opt_data_writeback:
1141 data_opt = EXT3_MOUNT_WRITEBACK_DATA;
1142 datacheck:
1143 if (is_remount) {
1144 if (test_opt(sb, DATA_FLAGS) == data_opt)
1145 break;
1146 ext3_msg(sb, KERN_ERR,
1147 "error: cannot change "
1148 "data mode on remount. The filesystem "
1149 "is mounted in data=%s mode and you "
1150 "try to remount it in data=%s mode.",
1151 data_mode_string(test_opt(sb,
1152 DATA_FLAGS)),
1153 data_mode_string(data_opt));
1154 return 0;
1155 } else {
1156 clear_opt(sbi->s_mount_opt, DATA_FLAGS);
1157 sbi->s_mount_opt |= data_opt;
1158 }
1159 break;
1160 case Opt_data_err_abort:
1161 set_opt(sbi->s_mount_opt, DATA_ERR_ABORT);
1162 break;
1163 case Opt_data_err_ignore:
1164 clear_opt(sbi->s_mount_opt, DATA_ERR_ABORT);
1165 break;
1166 #ifdef CONFIG_QUOTA
1167 case Opt_usrjquota:
1168 if (!set_qf_name(sb, USRQUOTA, &args[0]))
1169 return 0;
1170 break;
1171 case Opt_grpjquota:
1172 if (!set_qf_name(sb, GRPQUOTA, &args[0]))
1173 return 0;
1174 break;
1175 case Opt_offusrjquota:
1176 if (!clear_qf_name(sb, USRQUOTA))
1177 return 0;
1178 break;
1179 case Opt_offgrpjquota:
1180 if (!clear_qf_name(sb, GRPQUOTA))
1181 return 0;
1182 break;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times.
2012-12-26 5:04 [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times Chen Gang
@ 2012-12-27 8:06 ` Chen Gang
2012-12-31 12:06 ` Jan Kara
1 sibling, 0 replies; 7+ messages in thread
From: Chen Gang @ 2012-12-27 8:06 UTC (permalink / raw)
To: Theodore Ts'o, adilger.kernel; +Cc: akpm, jack, linux-ext4
Really (just as Theodore Ts'o said), for ext4 is the same thing as ext3 !
so please help check it by ext4 maintainers, or ext3 maintainers
if want to let me to send relative patch, please tell me, I will try.
:-)
thanks.
gchen
于 2012年12月26日 13:04, Chen Gang 写道:
> Hello Theodore Ts'o
>
> in fs/ext3/supper.c
> for function set_qf_name:
> sbi->s_qf_names[qtype] may already have owned a memory (line 919..925)
> we set sbi->s_qf_names[qtype] = qname directly without checking (line 926)
>
> for function clear_qf_name:
> we set sbi->s_qf_names[qtype] = NULL (line 942..952)
>
>
> for function parse_options:
> we can call set_qf_name or clear_qf_name with USR or GRP many times.
> we find parameters not mind whether they are repeated. (line 975..985)
> so we may call set_qf_name or clear_qf_name several times.
> also may first call set_qf_name, then call clear_qf_name.
>
> in this situation, we will get memory leak.
>
> please help check this suggestion whether valid (I find it by code review).
>
>
> regards.
>
> gchen.
>
> 901 static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
> 902 {
> 903 struct ext3_sb_info *sbi = EXT3_SB(sb);
> 904 char *qname;
> 905
> 906 if (sb_any_quota_loaded(sb) &&
> 907 !sbi->s_qf_names[qtype]) {
> 908 ext3_msg(sb, KERN_ERR,
> 909 "Cannot change journaled "
> 910 "quota options when quota turned on");
> 911 return 0;
> 912 }
> 913 qname = match_strdup(args);
> 914 if (!qname) {
> 915 ext3_msg(sb, KERN_ERR,
> 916 "Not enough memory for storing quotafile name");
> 917 return 0;
> 918 }
> 919 if (sbi->s_qf_names[qtype] &&
> 920 strcmp(sbi->s_qf_names[qtype], qname)) {
> 921 ext3_msg(sb, KERN_ERR,
> 922 "%s quota file already specified", QTYPE2NAME(qtype));
> 923 kfree(qname);
> 924 return 0;
> 925 }
> 926 sbi->s_qf_names[qtype] = qname;
> 927 if (strchr(sbi->s_qf_names[qtype], '/')) {
> 928 ext3_msg(sb, KERN_ERR,
> 929 "quotafile must be on filesystem root");
> 930 kfree(sbi->s_qf_names[qtype]);
> 931 sbi->s_qf_names[qtype] = NULL;
> 932 return 0;
> 933 }
> 934 set_opt(sbi->s_mount_opt, QUOTA);
> 935 return 1;
> 936 }
> 937
> 938 static int clear_qf_name(struct super_block *sb, int qtype) {
> 939
> 940 struct ext3_sb_info *sbi = EXT3_SB(sb);
> 941
> 942 if (sb_any_quota_loaded(sb) &&
> 943 sbi->s_qf_names[qtype]) {
> 944 ext3_msg(sb, KERN_ERR, "Cannot change journaled quota options"
> 945 " when quota turned on");
> 946 return 0;
> 947 }
> 948 /*
> 949 * The space will be released later when all options are confirmed
> 950 * to be correct
> 951 */
> 952 sbi->s_qf_names[qtype] = NULL;
> 953 return 1;
> 954 }
> 955 #endif
> 956
> 957 static int parse_options (char *options, struct super_block *sb,
> 958 unsigned int *inum, unsigned long *journal_devnum,
> 959 ext3_fsblk_t *n_blocks_count, int is_remount)
> 960 {
> 961 struct ext3_sb_info *sbi = EXT3_SB(sb);
> 962 char * p;
> 963 substring_t args[MAX_OPT_ARGS];
> 964 int data_opt = 0;
> 965 int option;
> 966 kuid_t uid;
> 967 kgid_t gid;
> 968 #ifdef CONFIG_QUOTA
> 969 int qfmt;
> 970 #endif
> 971
> 972 if (!options)
> 973 return 1;
> 974
> 975 while ((p = strsep (&options, ",")) != NULL) {
> 976 int token;
> 977 if (!*p)
> 978 continue;
> 979 /*
> 980 * Initialize args struct so we know whether arg was
> 981 * found; some options take optional arguments.
> 982 */
> 983 args[0].to = args[0].from = NULL;
> 984 token = match_token(p, tokens, args);
> 985 switch (token) {
> 986 case Opt_bsd_df:
> 987 clear_opt (sbi->s_mount_opt, MINIX_DF);
> 988 break;
> 989 case Opt_minix_df:
> 990 set_opt (sbi->s_mount_opt, MINIX_DF);
> 991 break;
> 992 case Opt_grpid:
> 993 set_opt (sbi->s_mount_opt, GRPID);
> 955 #endif
> 956
> 957 static int parse_options (char *options, struct super_block *sb,
> 958 unsigned int *inum, unsigned long *journal_devnum,
> 959 ext3_fsblk_t *n_blocks_count, int is_remount)
> 960 {
> 961 struct ext3_sb_info *sbi = EXT3_SB(sb);
> 962 char * p;
> 963 substring_t args[MAX_OPT_ARGS];
> 964 int data_opt = 0;
> 965 int option;
> 966 kuid_t uid;
> 967 kgid_t gid;
> 968 #ifdef CONFIG_QUOTA
> 969 int qfmt;
> 970 #endif
> 971
> 972 if (!options)
> 973 return 1;
> 974
> 975 while ((p = strsep (&options, ",")) != NULL) {
> 976 int token;
> 977 if (!*p)
> 978 continue;
> 979 /*
> 980 * Initialize args struct so we know whether arg was
> 981 * found; some options take optional arguments.
> 982 */
> 983 args[0].to = args[0].from = NULL;
> 984 token = match_token(p, tokens, args);
> 985 switch (token) {
> 986 case Opt_bsd_df:
> 987 clear_opt (sbi->s_mount_opt, MINIX_DF);
> 988 break;
> 989 case Opt_minix_df:
> 990 set_opt (sbi->s_mount_opt, MINIX_DF);
> 991 break;
> 992 case Opt_grpid:
> 993 set_opt (sbi->s_mount_opt, GRPID);
> 994 break;
> 995 case Opt_nogrpid:
> 996 clear_opt (sbi->s_mount_opt, GRPID);
> 997 break;
> 998 case Opt_resuid:
> 999 if (match_int(&args[0], &option))
> 1000 return 0;
> 1001 uid = make_kuid(current_user_ns(), option);
> 1002 if (!uid_valid(uid)) {
> 1003 ext3_msg(sb, KERN_ERR, "Invalid uid value %d", option);
> 1004 return 0;
> 1005
> 1006 }
> 1007 sbi->s_resuid = uid;
> 1008 break;
> 1009 case Opt_resgid:
> 1010 if (match_int(&args[0], &option))
> 1011 return 0;
> 1012 gid = make_kgid(current_user_ns(), option);
> 1013 if (!gid_valid(gid)) {
> 1014 ext3_msg(sb, KERN_ERR, "Invalid gid value %d", option);
> 1015 return 0;
> 1016 }
> 1017 sbi->s_resgid = gid;
> 1018 break;
> 1019 case Opt_sb:
> 1020 /* handled by get_sb_block() instead of here */
> 1021 /* *sb_block = match_int(&args[0]); */
> 1022 break;
> 1023 case Opt_err_panic:
> 1024 clear_opt (sbi->s_mount_opt, ERRORS_CONT);
> 1025 clear_opt (sbi->s_mount_opt, ERRORS_RO);
> 1026 set_opt (sbi->s_mount_opt, ERRORS_PANIC);
> 1027 break;
> 1028 case Opt_err_ro:
> 1029 clear_opt (sbi->s_mount_opt, ERRORS_CONT);
> 1030 clear_opt (sbi->s_mount_opt, ERRORS_PANIC);
> 1031 set_opt (sbi->s_mount_opt, ERRORS_RO);
> 1032 break;
> 1033 case Opt_err_cont:
> 1034 clear_opt (sbi->s_mount_opt, ERRORS_RO);
> 1035 clear_opt (sbi->s_mount_opt, ERRORS_PANIC);
> 1036 set_opt (sbi->s_mount_opt, ERRORS_CONT);
> 1037 break;
> 1038 case Opt_nouid32:
> 1039 set_opt (sbi->s_mount_opt, NO_UID32);
> 1040 break;
> 1041 case Opt_nocheck:
> 1042 clear_opt (sbi->s_mount_opt, CHECK);
> 1043 break;
> 1044 case Opt_debug:
> 1045 set_opt (sbi->s_mount_opt, DEBUG);
> 1046 break;
> 1047 case Opt_oldalloc:
> 1048 ext3_msg(sb, KERN_WARNING,
> 1049 "Ignoring deprecated oldalloc option");
> 1050 break;
> 1051 case Opt_orlov:
> 1052 ext3_msg(sb, KERN_WARNING,
> 1053 "Ignoring deprecated orlov option");
> 1054 break;
> 1055 #ifdef CONFIG_EXT3_FS_XATTR
> 1056 case Opt_user_xattr:
> 1057 set_opt (sbi->s_mount_opt, XATTR_USER);
> 1058 break;
> 1059 case Opt_nouser_xattr:
> 1060 clear_opt (sbi->s_mount_opt, XATTR_USER);
> 1061 break;
> 1062 #else
> 1063 case Opt_user_xattr:
> 1064 case Opt_nouser_xattr:
> 1065 ext3_msg(sb, KERN_INFO,
> 1066 "(no)user_xattr options not supported");
> 1067 break;
> 1068 #endif
> 1069 #ifdef CONFIG_EXT3_FS_POSIX_ACL
> 1070 case Opt_acl:
> 1071 set_opt(sbi->s_mount_opt, POSIX_ACL);
> 1072 break;
> 1073 case Opt_noacl:
> 1074 clear_opt(sbi->s_mount_opt, POSIX_ACL);
> 1075 break;
> 1076 #else
> 1077 case Opt_acl:
> 1078 case Opt_noacl:
> 1079 ext3_msg(sb, KERN_INFO,
> 1080 "(no)acl options not supported");
> 1081 break;
> 1082 #endif
> 1083 case Opt_reservation:
> 1084 set_opt(sbi->s_mount_opt, RESERVATION);
> 1085 break;
> 1086 case Opt_noreservation:
> 1087 clear_opt(sbi->s_mount_opt, RESERVATION);
> 1088 break;
> 1089 case Opt_journal_update:
> 1090 /* @@@ FIXME */
> 1091 /* Eventually we will want to be able to create
> 1092 a journal file here. For now, only allow the
> 1093 user to specify an existing inode to be the
> 1094 journal file. */
> 1095 if (is_remount) {
> 1096 ext3_msg(sb, KERN_ERR, "error: cannot specify "
> 1097 "journal on remount");
> 1098 return 0;
> 1099 }
> 1100 set_opt (sbi->s_mount_opt, UPDATE_JOURNAL);
> 1101 break;
> 1102 case Opt_journal_inum:
> 1103 if (is_remount) {
> 1104 ext3_msg(sb, KERN_ERR, "error: cannot specify "
> 1105 "journal on remount");
> 1106 return 0;
> 1107 }
> 1108 if (match_int(&args[0], &option))
> 1109 return 0;
> 1110 *inum = option;
> 1111 break;
> 1112 case Opt_journal_dev:
> 1113 if (is_remount) {
> 1114 ext3_msg(sb, KERN_ERR, "error: cannot specify "
> 1115 "journal on remount");
> 1116 return 0;
> 1117 }
> 1118 if (match_int(&args[0], &option))
> 1119 return 0;
> 1120 *journal_devnum = option;
> 1121 break;
> 1122 case Opt_noload:
> 1123 set_opt (sbi->s_mount_opt, NOLOAD);
> 1124 break;
> 1125 case Opt_commit:
> 1126 if (match_int(&args[0], &option))
> 1127 return 0;
> 1128 if (option < 0)
> 1129 return 0;
> 1130 if (option == 0)
> 1131 option = JBD_DEFAULT_MAX_COMMIT_AGE;
> 1132 sbi->s_commit_interval = HZ * option;
> 1133 break;
> 1134 case Opt_data_journal:
> 1135 data_opt = EXT3_MOUNT_JOURNAL_DATA;
> 1136 goto datacheck;
> 1137 case Opt_data_ordered:
> 1138 data_opt = EXT3_MOUNT_ORDERED_DATA;
> 1139 goto datacheck;
> 1140 case Opt_data_writeback:
> 1141 data_opt = EXT3_MOUNT_WRITEBACK_DATA;
> 1142 datacheck:
> 1143 if (is_remount) {
> 1144 if (test_opt(sb, DATA_FLAGS) == data_opt)
> 1145 break;
> 1146 ext3_msg(sb, KERN_ERR,
> 1147 "error: cannot change "
> 1148 "data mode on remount. The filesystem "
> 1149 "is mounted in data=%s mode and you "
> 1150 "try to remount it in data=%s mode.",
> 1151 data_mode_string(test_opt(sb,
> 1152 DATA_FLAGS)),
> 1153 data_mode_string(data_opt));
> 1154 return 0;
> 1155 } else {
> 1156 clear_opt(sbi->s_mount_opt, DATA_FLAGS);
> 1157 sbi->s_mount_opt |= data_opt;
> 1158 }
> 1159 break;
> 1160 case Opt_data_err_abort:
> 1161 set_opt(sbi->s_mount_opt, DATA_ERR_ABORT);
> 1162 break;
> 1163 case Opt_data_err_ignore:
> 1164 clear_opt(sbi->s_mount_opt, DATA_ERR_ABORT);
> 1165 break;
> 1166 #ifdef CONFIG_QUOTA
> 1167 case Opt_usrjquota:
> 1168 if (!set_qf_name(sb, USRQUOTA, &args[0]))
> 1169 return 0;
> 1170 break;
> 1171 case Opt_grpjquota:
> 1172 if (!set_qf_name(sb, GRPQUOTA, &args[0]))
> 1173 return 0;
> 1174 break;
> 1175 case Opt_offusrjquota:
> 1176 if (!clear_qf_name(sb, USRQUOTA))
> 1177 return 0;
> 1178 break;
> 1179 case Opt_offgrpjquota:
> 1180 if (!clear_qf_name(sb, GRPQUOTA))
> 1181 return 0;
> 1182 break;
>
--
Chen Gang
Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times.
2012-12-26 5:04 [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times Chen Gang
2012-12-27 8:06 ` Chen Gang
@ 2012-12-31 12:06 ` Jan Kara
2013-01-04 5:39 ` Chen Gang
1 sibling, 1 reply; 7+ messages in thread
From: Jan Kara @ 2012-12-31 12:06 UTC (permalink / raw)
To: Chen Gang; +Cc: Theodore Ts'o, jack, akpm, linux-ext4
[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]
On Wed 26-12-12 13:04:59, Chen Gang wrote:
> Hello Theodore Ts'o
>
> in fs/ext3/supper.c
> for function set_qf_name:
> sbi->s_qf_names[qtype] may already have owned a memory (line 919..925)
> we set sbi->s_qf_names[qtype] = qname directly without checking (line 926)
>
> for function clear_qf_name:
> we set sbi->s_qf_names[qtype] = NULL (line 942..952)
>
>
> for function parse_options:
> we can call set_qf_name or clear_qf_name with USR or GRP many times.
> we find parameters not mind whether they are repeated. (line 975..985)
> so we may call set_qf_name or clear_qf_name several times.
> also may first call set_qf_name, then call clear_qf_name.
>
> in this situation, we will get memory leak.
>
> please help check this suggestion whether valid (I find it by code review).
Thanks for report. Yes, memory leak seems to be possible. Attached patch
should fix it, I have added it to my tree.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
[-- Attachment #2: 0001-ext3-Fix-memory-leak-when-quota-options-are-specifie.patch --]
[-- Type: text/x-patch, Size: 1359 bytes --]
>From 59cefce80eb3e081d0dbef335843340512aff79e Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 31 Dec 2012 12:38:36 +0100
Subject: [PATCH] ext3: Fix memory leak when quota options are specified multiple times
When usrjquota or grpjquota mount options are specified several times
with the same file name, we leak memory storing the names. Free the
memory correctly.
Reported-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/ext3/super.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 6e50223..b36cea9 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -916,12 +916,16 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
"Not enough memory for storing quotafile name");
return 0;
}
- if (sbi->s_qf_names[qtype] &&
- strcmp(sbi->s_qf_names[qtype], qname)) {
- ext3_msg(sb, KERN_ERR,
- "%s quota file already specified", QTYPE2NAME(qtype));
+ if (sbi->s_qf_names[qtype]) {
+ int same = !strcmp(sbi->s_qf_names[qtype], qname);
+
kfree(qname);
- return 0;
+ if (!same) {
+ ext3_msg(sb, KERN_ERR,
+ "%s quota file already specified",
+ QTYPE2NAME(qtype));
+ }
+ return same;
}
sbi->s_qf_names[qtype] = qname;
if (strchr(sbi->s_qf_names[qtype], '/')) {
--
1.7.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times.
2012-12-31 12:06 ` Jan Kara
@ 2013-01-04 5:39 ` Chen Gang
2013-01-07 19:54 ` Jan Kara
0 siblings, 1 reply; 7+ messages in thread
From: Chen Gang @ 2013-01-04 5:39 UTC (permalink / raw)
To: Jan Kara; +Cc: Theodore Ts'o, akpm, linux-ext4
于 2012年12月31日 20:06, Jan Kara 写道:
> Thanks for report. Yes, memory leak seems to be possible. Attached patch
> should fix it, I have added it to my tree.
>
> Honza
>
after read your patch, today (although I am not a reviewer).
is it a possible situation like this (just my imagination):
1st let it call clear_qf_name successfully.
2nd let it call set_qf_name successfully.
3rd call clear_qf_name again ?
...
if this situation is possible.
I guess the memory still leak, after apply your patch.
if what I guess is true
it seems we have to duplicate memory for old_opts in ext3_remount.
and for clear_qf_name, set_qf_name, ext3_remount:
use normal memory allocation and free ways instead of current using.
Regards
--
Chen Gang
Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times.
2013-01-04 5:39 ` Chen Gang
@ 2013-01-07 19:54 ` Jan Kara
2013-01-08 3:13 ` Chen Gang
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2013-01-07 19:54 UTC (permalink / raw)
To: Chen Gang; +Cc: Jan Kara, Theodore Ts'o, akpm, linux-ext4
[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]
On Fri 04-01-13 13:39:37, Chen Gang wrote:
> 于 2012年12月31日 20:06, Jan Kara 写道:
> > Thanks for report. Yes, memory leak seems to be possible. Attached patch
> > should fix it, I have added it to my tree.
> >
> > Honza
> >
>
> after read your patch, today (although I am not a reviewer).
> is it a possible situation like this (just my imagination):
> 1st let it call clear_qf_name successfully.
> 2nd let it call set_qf_name successfully.
> 3rd call clear_qf_name again ?
Although this is not likely in practice, if you specify mount options
like usrjquota=aquota.user,usrjquota=,usrjquota=aquota.user it can happen.
Thanks for spotting this.
> ...
> if this situation is possible.
> I guess the memory still leak, after apply your patch.
>
> if what I guess is true
> it seems we have to duplicate memory for old_opts in ext3_remount.
> and for clear_qf_name, set_qf_name, ext3_remount:
> use normal memory allocation and free ways instead of current using.
Yes. Updated patch attached.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
[-- Attachment #2: 0001-ext3-Fix-memory-leak-when-quota-options-are-specifie.patch --]
[-- Type: text/x-patch, Size: 3346 bytes --]
>From c36504046b93419fba7ec0029a43afd2bbdd6c48 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 31 Dec 2012 12:38:36 +0100
Subject: [PATCH] ext3: Fix memory leak when quota options are specified multiple times
When usrjquota or grpjquota mount options are specified several times,
we leak memory storing the names. Free the memory correctly.
Reported-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/ext3/super.c | 51 ++++++++++++++++++++++++++++++---------------------
1 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 6e50223..0926fe4 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -916,21 +916,24 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
"Not enough memory for storing quotafile name");
return 0;
}
- if (sbi->s_qf_names[qtype] &&
- strcmp(sbi->s_qf_names[qtype], qname)) {
- ext3_msg(sb, KERN_ERR,
- "%s quota file already specified", QTYPE2NAME(qtype));
+ if (sbi->s_qf_names[qtype]) {
+ int same = !strcmp(sbi->s_qf_names[qtype], qname);
+
kfree(qname);
- return 0;
+ if (!same) {
+ ext3_msg(sb, KERN_ERR,
+ "%s quota file already specified",
+ QTYPE2NAME(qtype));
+ }
+ return same;
}
- sbi->s_qf_names[qtype] = qname;
- if (strchr(sbi->s_qf_names[qtype], '/')) {
+ if (strchr(qname, '/')) {
ext3_msg(sb, KERN_ERR,
"quotafile must be on filesystem root");
- kfree(sbi->s_qf_names[qtype]);
- sbi->s_qf_names[qtype] = NULL;
+ kfree(qname);
return 0;
}
+ sbi->s_qf_names[qtype] = qname;
set_opt(sbi->s_mount_opt, QUOTA);
return 1;
}
@@ -945,11 +948,10 @@ static int clear_qf_name(struct super_block *sb, int qtype) {
" when quota turned on");
return 0;
}
- /*
- * The space will be released later when all options are confirmed
- * to be correct
- */
- sbi->s_qf_names[qtype] = NULL;
+ if (sbi->s_qf_names[qtype]) {
+ kfree(sbi->s_qf_names[qtype]);
+ sbi->s_qf_names[qtype] = NULL;
+ }
return 1;
}
#endif
@@ -2605,7 +2607,18 @@ static int ext3_remount (struct super_block * sb, int * flags, char * data)
#ifdef CONFIG_QUOTA
old_opts.s_jquota_fmt = sbi->s_jquota_fmt;
for (i = 0; i < MAXQUOTAS; i++)
- old_opts.s_qf_names[i] = sbi->s_qf_names[i];
+ if (sbi->s_qf_names[i]) {
+ old_opts.s_qf_names[i] = kstrdup(sbi->s_qf_names[i],
+ GFP_KERNEL);
+ if (!old_opts.s_qf_names[i]) {
+ int j;
+
+ for (j = 0; j < i; j++)
+ kfree(old_opts.s_qf_names[j]);
+ return -ENOMEM;
+ }
+ } else
+ old_opts.s_qf_names[i] = NULL;
#endif
/*
@@ -2698,9 +2711,7 @@ static int ext3_remount (struct super_block * sb, int * flags, char * data)
#ifdef CONFIG_QUOTA
/* Release old quota file names */
for (i = 0; i < MAXQUOTAS; i++)
- if (old_opts.s_qf_names[i] &&
- old_opts.s_qf_names[i] != sbi->s_qf_names[i])
- kfree(old_opts.s_qf_names[i]);
+ kfree(old_opts.s_qf_names[i]);
#endif
if (enable_quota)
dquot_resume(sb, -1);
@@ -2714,9 +2725,7 @@ restore_opts:
#ifdef CONFIG_QUOTA
sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
for (i = 0; i < MAXQUOTAS; i++) {
- if (sbi->s_qf_names[i] &&
- old_opts.s_qf_names[i] != sbi->s_qf_names[i])
- kfree(sbi->s_qf_names[i]);
+ kfree(sbi->s_qf_names[i]);
sbi->s_qf_names[i] = old_opts.s_qf_names[i];
}
#endif
--
1.7.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times.
2013-01-07 19:54 ` Jan Kara
@ 2013-01-08 3:13 ` Chen Gang
2013-01-14 2:29 ` Chen Gang
0 siblings, 1 reply; 7+ messages in thread
From: Chen Gang @ 2013-01-08 3:13 UTC (permalink / raw)
To: Jan Kara; +Cc: Theodore Ts'o, akpm, linux-ext4
于 2013年01月08日 03:54, Jan Kara 写道:
> On Fri 04-01-13 13:39:37, Chen Gang wrote:
>> if what I guess is true
>> it seems we have to duplicate memory for old_opts in ext3_remount.
>> and for clear_qf_name, set_qf_name, ext3_remount:
>> use normal memory allocation and free ways instead of current using.
> Yes. Updated patch attached.
>
at least for me, Jan Kara's patch is ok. thanks.
welcome another members (especially Theodore Ts'o) to giving confirmation or completion.
also welcome to checking this suggestion whether valid for fs/ext4.
--
Chen Gang
Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times.
2013-01-08 3:13 ` Chen Gang
@ 2013-01-14 2:29 ` Chen Gang
0 siblings, 0 replies; 7+ messages in thread
From: Chen Gang @ 2013-01-14 2:29 UTC (permalink / raw)
To: Jan Kara; +Cc: Theodore Ts'o, akpm, linux-ext4
于 2013年01月08日 11:13, Chen Gang 写道:
> 于 2013年01月08日 03:54, Jan Kara 写道:
>> > On Fri 04-01-13 13:39:37, Chen Gang wrote:
>>> >> if what I guess is true
>>> >> it seems we have to duplicate memory for old_opts in ext3_remount.
>>> >> and for clear_qf_name, set_qf_name, ext3_remount:
>>> >> use normal memory allocation and free ways instead of current using.
>> > Yes. Updated patch attached.
>> >
> at least for me, Jan Kara's patch is ok. thanks.
>
> welcome another members (especially Theodore Ts'o) to giving confirmation or completion.
> also welcome to checking this suggestion whether valid for fs/ext4.
I will send a relative patch for fs/ext4, just according to Jan Kara's patch.
I will send it within 2 weeks (excuse me, I have to do another things this week).
welcome any additional suggestions and completions.
thanks.
--
Chen Gang
Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-01-14 2:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-26 5:04 [Suggestion] fs/ext3: memory leak by calling set_qf_name or clear_qf_name, many times Chen Gang
2012-12-27 8:06 ` Chen Gang
2012-12-31 12:06 ` Jan Kara
2013-01-04 5:39 ` Chen Gang
2013-01-07 19:54 ` Jan Kara
2013-01-08 3:13 ` Chen Gang
2013-01-14 2:29 ` Chen Gang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).