* [bug report] NFSD: Convert the filecache to use rhashtable
@ 2022-07-06 14:33 Dan Carpenter
2022-07-06 14:36 ` Chuck Lever III
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2022-07-06 14:33 UTC (permalink / raw)
To: chuck.lever; +Cc: linux-nfs
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
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] NFSD: Convert the filecache to use rhashtable
2022-07-06 14:33 [bug report] NFSD: Convert the filecache to use rhashtable Dan Carpenter
@ 2022-07-06 14:36 ` Chuck Lever III
0 siblings, 0 replies; 2+ messages in thread
From: Chuck Lever III @ 2022-07-06 14:36 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Linux NFS Mailing List
> On Jul 6, 2022, at 10:33 AM, Dan Carpenter <dan.carpenter@oracle.com> 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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-07-06 14:36 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-06 14:33 [bug report] NFSD: Convert the filecache to use rhashtable Dan Carpenter
2022-07-06 14:36 ` Chuck Lever III
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.