> On Jul 6, 2022, at 10:33 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Chuck Lever,
>
> The patch 125b58c13f71: "NFSD: Convert the filecache to use
> rhashtable" from Jun 28, 2022, leads to the following Smatch static
> checker warning:
>
> fs/nfsd/filecache.c:1117 nfsd_file_do_acquire()
> warn: 'new' was already freed.
>
> fs/nfsd/filecache.c
> 1066 static __be32
> 1067 nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 1068 unsigned int may_flags, struct nfsd_file **pnf, bool open)
> 1069 {
> 1070 struct nfsd_file_lookup_key key = {
> 1071 .type = NFSD_FILE_KEY_FULL,
> 1072 .need = may_flags & NFSD_FILE_MAY_MASK,
> 1073 .net = SVC_NET(rqstp),
> 1074 .cred = current_cred(),
> 1075 };
> 1076 struct nfsd_file *nf, *new;
> 1077 bool retry = true;
> 1078 __be32 status;
> 1079
> 1080 status = fh_verify(rqstp, fhp, S_IFREG,
> 1081 may_flags|NFSD_MAY_OWNER_OVERRIDE);
> 1082 if (status != nfs_ok)
> 1083 return status;
> 1084 key.inode = d_inode(fhp->fh_dentry);
> 1085
> 1086 retry:
> 1087 /* Avoid allocation if the item is already in cache */
> 1088 nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> 1089 nfsd_file_rhash_params);
> 1090 if (nf)
> 1091 nf = nfsd_file_get(nf);
> 1092 if (nf)
> 1093 goto wait_for_construction;
> 1094
> 1095 new = nfsd_file_alloc(&key, may_flags);
> 1096 if (!new) {
> 1097 status = nfserr_jukebox;
> 1098 goto out_status;
> 1099 }
> 1100
> 1101 nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> 1102 &key, &new->nf_rhash,
> 1103 nfsd_file_rhash_params);
> 1104 if (!nf) {
> 1105 nf = new;
> 1106 goto open_file;
> 1107 }
> 1108 nfsd_file_slab_free(&new->nf_rcu);
> ^^^^^^^^^^^
> This frees "new".
>
> 1109 if (IS_ERR(nf)) {
> 1110 trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
> 1111 nf = NULL;
> 1112 status = nfserr_jukebox;
> 1113 goto out_status;
> 1114 }
> 1115 nf = nfsd_file_get(nf);
> 1116 if (nf == NULL) {
> --> 1117 nf = new;
> ^^^
> Use after free
Ugh. I wonder why I haven't hit this already.
Thanks Dan!
> 1118 goto open_file;
> 1119 }
> 1120
> 1121 wait_for_construction:
> 1122 wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> 1123
> 1124 /* Did construction of this file fail? */
> 1125 if (!test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> 1126 trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf);
> 1127 if (!retry) {
> 1128 status = nfserr_jukebox;
> 1129 goto out;
> 1130 }
> 1131 retry = false;
> 1132 nfsd_file_put_noref(nf);
> 1133 goto retry;
> 1134 }
> 1135
> 1136 nfsd_file_lru_remove(nf);
> 1137 this_cpu_inc(nfsd_file_cache_hits);
> 1138
> 1139 if (!(may_flags & NFSD_MAY_NOT_BREAK_LEASE)) {
> 1140 bool write = (may_flags & NFSD_MAY_WRITE);
> 1141
> 1142 if (test_bit(NFSD_FILE_BREAK_READ, &nf->nf_flags) ||
> 1143 (test_bit(NFSD_FILE_BREAK_WRITE, &nf->nf_flags) && write)) {
> 1144 status = nfserrno(nfsd_open_break_lease(
> 1145 file_inode(nf->nf_file), may_flags));
> 1146 if (status == nfs_ok) {
> 1147 clear_bit(NFSD_FILE_BREAK_READ, &nf->nf_flags);
> 1148 if (write)
> 1149 clear_bit(NFSD_FILE_BREAK_WRITE,
> 1150 &nf->nf_flags);
> 1151 }
> 1152 }
> 1153 }
> 1154 out:
> 1155 if (status == nfs_ok) {
> 1156 if (open)
> 1157 this_cpu_inc(nfsd_file_acquisitions);
> 1158 *pnf = nf;
> 1159 } else {
> 1160 nfsd_file_put(nf);
> 1161 nf = NULL;
> 1162 }
> 1163
> 1164 out_status:
> 1165 if (open)
> 1166 trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status);
> 1167 return status;
> 1168
> 1169 open_file:
> 1170 trace_nfsd_file_alloc(nf);
> 1171 nf->nf_mark = nfsd_file_mark_find_or_create(nf);
> 1172 if (nf->nf_mark) {
> 1173 if (open) {
> 1174 status = nfsd_open_verified(rqstp, fhp, may_flags,
> 1175 &nf->nf_file);
> 1176 trace_nfsd_file_open(nf, status);
> 1177 } else
> 1178 status = nfs_ok;
> 1179 } else
> 1180 status = nfserr_jukebox;
> 1181 /*
> 1182 * If construction failed, or we raced with a call to unlink()
> 1183 * then unhash.
> 1184 */
> 1185 if (status != nfs_ok || key.inode->i_nlink == 0)
> 1186 if (nfsd_file_unhash(nf))
> 1187 nfsd_file_put_noref(nf);
> 1188 clear_bit_unlock(NFSD_FILE_PENDING, &nf->nf_flags);
> 1189 smp_mb__after_atomic();
> 1190 wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
> 1191 goto out;
> 1192 }
>
> regards,
> dan carpenter
--
Chuck Lever
