Re: [bug report] NFSD: Convert the filecache to use rhashtable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> 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







[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux