Skip to content

Commit 4acb665

Browse files
dhowellsbrauner
dhowells
authored and
brauner
committed
netfs: Work around recursion by abandoning retry if nothing read
syzkaller reported recursion with a loop of three calls (netfs_rreq_assess, netfs_retry_reads and netfs_rreq_terminated) hitting the limit of the stack during an unbuffered or direct I/O read. There are a number of issues: (1) There is no limit on the number of retries. (2) A subrequest is supposed to be abandoned if it does not transfer anything (NETFS_SREQ_NO_PROGRESS), but that isn't checked under all circumstances. (3) The actual root cause, which is this: if (atomic_dec_and_test(&rreq->nr_outstanding)) netfs_rreq_terminated(rreq, ...); When we do a retry, we bump the rreq->nr_outstanding counter to prevent the final cleanup phase running before we've finished dispatching the retries. The problem is if we hit 0, we have to do the cleanup phase - but we're in the cleanup phase and end up repeating the retry cycle, hence the recursion. Work around the problem by limiting the number of retries. This is based on Lizhi Xu's patch[1], and makes the following changes: (1) Replace NETFS_SREQ_NO_PROGRESS with NETFS_SREQ_MADE_PROGRESS and make the filesystem set it if it managed to read or write at least one byte of data. Clear this bit before issuing a subrequest. (2) Add a ->retry_count member to the subrequest and increment it any time we do a retry. (3) Remove the NETFS_SREQ_RETRYING flag as it is superfluous with ->retry_count. If the latter is non-zero, we're doing a retry. (4) Abandon a subrequest if retry_count is non-zero and we made no progress. (5) Use ->retry_count in both the write-side and the read-size. [?] Question: Should I set a hard limit on retry_count in both read and write? Say it hits 50, we always abandon it. The problem is that these changes only mitigate the issue. As long as it made at least one byte of progress, the recursion is still an issue. This patch mitigates the problem, but does not fix the underlying cause. I have patches that will do that, but it's an intrusive fix that's currently pending for the next merge window. The oops generated by KASAN looks something like: BUG: TASK stack guard page was hit at ffffc9000482ff48 (stack is ffffc90004830000..ffffc90004838000) Oops: stack guard page: 0000 [#1] PREEMPT SMP KASAN NOPTI ... RIP: 0010:mark_lock+0x25/0xc60 kernel/locking/lockdep.c:4686 ... mark_usage kernel/locking/lockdep.c:4646 [inline] __lock_acquire+0x906/0x3ce0 kernel/locking/lockdep.c:5156 lock_acquire.part.0+0x11b/0x380 kernel/locking/lockdep.c:5825 local_lock_acquire include/linux/local_lock_internal.h:29 [inline] ___slab_alloc+0x123/0x1880 mm/slub.c:3695 __slab_alloc.constprop.0+0x56/0xb0 mm/slub.c:3908 __slab_alloc_node mm/slub.c:3961 [inline] slab_alloc_node mm/slub.c:4122 [inline] kmem_cache_alloc_noprof+0x2a7/0x2f0 mm/slub.c:4141 radix_tree_node_alloc.constprop.0+0x1e8/0x350 lib/radix-tree.c:253 idr_get_free+0x528/0xa40 lib/radix-tree.c:1506 idr_alloc_u32+0x191/0x2f0 lib/idr.c:46 idr_alloc+0xc1/0x130 lib/idr.c:87 p9_tag_alloc+0x394/0x870 net/9p/client.c:321 p9_client_prepare_req+0x19f/0x4d0 net/9p/client.c:644 p9_client_zc_rpc.constprop.0+0x105/0x880 net/9p/client.c:793 p9_client_read_once+0x443/0x820 net/9p/client.c:1570 p9_client_read+0x13f/0x1b0 net/9p/client.c:1534 v9fs_issue_read+0x115/0x310 fs/9p/vfs_addr.c:74 netfs_retry_read_subrequests fs/netfs/read_retry.c:60 [inline] netfs_retry_reads+0x153a/0x1d00 fs/netfs/read_retry.c:232 netfs_rreq_assess+0x5d3/0x870 fs/netfs/read_collect.c:371 netfs_rreq_terminated+0xe5/0x110 fs/netfs/read_collect.c:407 netfs_retry_reads+0x155e/0x1d00 fs/netfs/read_retry.c:235 netfs_rreq_assess+0x5d3/0x870 fs/netfs/read_collect.c:371 netfs_rreq_terminated+0xe5/0x110 fs/netfs/read_collect.c:407 netfs_retry_reads+0x155e/0x1d00 fs/netfs/read_retry.c:235 netfs_rreq_assess+0x5d3/0x870 fs/netfs/read_collect.c:371 ... netfs_rreq_terminated+0xe5/0x110 fs/netfs/read_collect.c:407 netfs_retry_reads+0x155e/0x1d00 fs/netfs/read_retry.c:235 netfs_rreq_assess+0x5d3/0x870 fs/netfs/read_collect.c:371 netfs_rreq_terminated+0xe5/0x110 fs/netfs/read_collect.c:407 netfs_retry_reads+0x155e/0x1d00 fs/netfs/read_retry.c:235 netfs_rreq_assess+0x5d3/0x870 fs/netfs/read_collect.c:371 netfs_rreq_terminated+0xe5/0x110 fs/netfs/read_collect.c:407 netfs_dispatch_unbuffered_reads fs/netfs/direct_read.c:103 [inline] netfs_unbuffered_read fs/netfs/direct_read.c:127 [inline] netfs_unbuffered_read_iter_locked+0x12f6/0x19b0 fs/netfs/direct_read.c:221 netfs_unbuffered_read_iter+0xc5/0x100 fs/netfs/direct_read.c:256 v9fs_file_read_iter+0xbf/0x100 fs/9p/vfs_file.c:361 do_iter_readv_writev+0x614/0x7f0 fs/read_write.c:832 vfs_readv+0x4cf/0x890 fs/read_write.c:1025 do_preadv fs/read_write.c:1142 [inline] __do_sys_preadv fs/read_write.c:1192 [inline] __se_sys_preadv fs/read_write.c:1187 [inline] __x64_sys_preadv+0x22d/0x310 fs/read_write.c:1187 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83 Fixes: ee4cdf7 ("netfs: Speed up buffered reading") Closes: https://syzkaller.appspot.com/bug?extid=1fc6f64c40a9d143cfb6 Signed-off-by: David Howells <dhowells@redhat.com> Link: https://lore.kernel.org/r/20241108034020.3695718-1-lizhi.xu@windriver.com/ [1] Link: https://lore.kernel.org/r/20241213135013.2964079-9-dhowells@redhat.com Tested-by: syzbot+885c03ad650731743489@syzkaller.appspotmail.com Suggested-by: Lizhi Xu <lizhi.xu@windriver.com> cc: Dominique Martinet <asmadeus@codewreck.org> cc: Jeff Layton <jlayton@kernel.org> cc: v9fs@lists.linux.dev cc: netfs@lists.linux.dev cc: linux-fsdevel@vger.kernel.org Reported-by: syzbot+885c03ad650731743489@syzkaller.appspotmail.com Signed-off-by: Christian Brauner <brauner@kernel.org>
1 parent aa39564 commit 4acb665

File tree

9 files changed

+44
-23
lines changed

9 files changed

+44
-23
lines changed

‎fs/9p/vfs_addr.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ static void v9fs_issue_write(struct netfs_io_subrequest *subreq)
5757
int err, len;
5858

5959
len = p9_client_write(fid, subreq->start, &subreq->io_iter, &err);
60+
if (len > 0)
61+
__set_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
6062
netfs_write_subrequest_terminated(subreq, len ?: err, false);
6163
}
6264

@@ -80,8 +82,10 @@ static void v9fs_issue_read(struct netfs_io_subrequest *subreq)
8082
if (pos + total >= i_size_read(rreq->inode))
8183
__set_bit(NETFS_SREQ_HIT_EOF, &subreq->flags);
8284

83-
if (!err)
85+
if (!err) {
8486
subreq->transferred += total;
87+
__set_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
88+
}
8589

8690
netfs_read_subreq_terminated(subreq, err, false);
8791
}

‎fs/afs/write.c

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ static void afs_issue_write_worker(struct work_struct *work)
122122
if (subreq->debug_index == 3)
123123
return netfs_write_subrequest_terminated(subreq, -ENOANO, false);
124124

125-
if (!test_bit(NETFS_SREQ_RETRYING, &subreq->flags)) {
125+
if (!subreq->retry_count) {
126126
set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
127127
return netfs_write_subrequest_terminated(subreq, -EAGAIN, false);
128128
}
@@ -149,6 +149,9 @@ static void afs_issue_write_worker(struct work_struct *work)
149149
afs_wait_for_operation(op);
150150
ret = afs_put_operation(op);
151151
switch (ret) {
152+
case 0:
153+
__set_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
154+
break;
152155
case -EACCES:
153156
case -EPERM:
154157
case -ENOKEY:

‎fs/netfs/read_collect.c

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,7 @@ void netfs_read_subreq_progress(struct netfs_io_subrequest *subreq,
438438
rreq->origin == NETFS_READPAGE ||
439439
rreq->origin == NETFS_READ_FOR_WRITE)) {
440440
netfs_consume_read_data(subreq, was_async);
441-
__clear_bit(NETFS_SREQ_NO_PROGRESS, &subreq->flags);
441+
__set_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
442442
}
443443
}
444444
EXPORT_SYMBOL(netfs_read_subreq_progress);
@@ -497,7 +497,7 @@ void netfs_read_subreq_terminated(struct netfs_io_subrequest *subreq,
497497
rreq->origin == NETFS_READPAGE ||
498498
rreq->origin == NETFS_READ_FOR_WRITE)) {
499499
netfs_consume_read_data(subreq, was_async);
500-
__clear_bit(NETFS_SREQ_NO_PROGRESS, &subreq->flags);
500+
__set_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
501501
}
502502
rreq->transferred += subreq->transferred;
503503
}
@@ -511,10 +511,13 @@ void netfs_read_subreq_terminated(struct netfs_io_subrequest *subreq,
511511
} else {
512512
trace_netfs_sreq(subreq, netfs_sreq_trace_short);
513513
if (subreq->transferred > subreq->consumed) {
514-
__set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
515-
__clear_bit(NETFS_SREQ_NO_PROGRESS, &subreq->flags);
516-
set_bit(NETFS_RREQ_NEED_RETRY, &rreq->flags);
517-
} else if (!__test_and_set_bit(NETFS_SREQ_NO_PROGRESS, &subreq->flags)) {
514+
/* If we didn't read new data, abandon retry. */
515+
if (subreq->retry_count &&
516+
test_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags)) {
517+
__set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
518+
set_bit(NETFS_RREQ_NEED_RETRY, &rreq->flags);
519+
}
520+
} else if (test_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags)) {
518521
__set_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
519522
set_bit(NETFS_RREQ_NEED_RETRY, &rreq->flags);
520523
} else {

‎fs/netfs/read_retry.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
5656
if (test_bit(NETFS_SREQ_FAILED, &subreq->flags))
5757
break;
5858
if (__test_and_clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags)) {
59+
__clear_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
60+
subreq->retry_count++;
5961
netfs_reset_iter(subreq);
6062
netfs_reissue_read(rreq, subreq);
6163
}
@@ -137,7 +139,8 @@ static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
137139
stream0->sreq_max_len = subreq->len;
138140

139141
__clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
140-
__set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
142+
__clear_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
143+
subreq->retry_count++;
141144

142145
spin_lock_bh(&rreq->lock);
143146
list_add_tail(&subreq->rreq_link, &rreq->subrequests);
@@ -213,7 +216,6 @@ static void netfs_retry_read_subrequests(struct netfs_io_request *rreq)
213216
subreq->error = -ENOMEM;
214217
__clear_bit(NETFS_SREQ_FAILED, &subreq->flags);
215218
__clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
216-
__clear_bit(NETFS_SREQ_RETRYING, &subreq->flags);
217219
}
218220
spin_lock_bh(&rreq->lock);
219221
list_splice_tail_init(&queue, &rreq->subrequests);

‎fs/netfs/write_collect.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,6 @@ static void netfs_retry_write_stream(struct netfs_io_request *wreq,
179179
struct iov_iter source = subreq->io_iter;
180180

181181
iov_iter_revert(&source, subreq->len - source.count);
182-
__set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
183182
netfs_get_subrequest(subreq, netfs_sreq_trace_get_resubmit);
184183
netfs_reissue_write(stream, subreq, &source);
185184
}
@@ -234,7 +233,7 @@ static void netfs_retry_write_stream(struct netfs_io_request *wreq,
234233
/* Renegotiate max_len (wsize) */
235234
trace_netfs_sreq(subreq, netfs_sreq_trace_retry);
236235
__clear_bit(NETFS_SREQ_NEED_RETRY, &subreq->flags);
237-
__set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
236+
subreq->retry_count++;
238237
stream->prepare_write(subreq);
239238

240239
part = min(len, stream->sreq_max_len);
@@ -279,7 +278,7 @@ static void netfs_retry_write_stream(struct netfs_io_request *wreq,
279278
subreq->start = start;
280279
subreq->debug_index = atomic_inc_return(&wreq->subreq_counter);
281280
subreq->stream_nr = to->stream_nr;
282-
__set_bit(NETFS_SREQ_RETRYING, &subreq->flags);
281+
subreq->retry_count = 1;
283282

284283
trace_netfs_sreq_ref(wreq->debug_id, subreq->debug_index,
285284
refcount_read(&subreq->ref),

‎fs/netfs/write_issue.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,8 @@ void netfs_reissue_write(struct netfs_io_stream *stream,
244244
iov_iter_advance(source, size);
245245
iov_iter_truncate(&subreq->io_iter, size);
246246

247+
subreq->retry_count++;
248+
__clear_bit(NETFS_SREQ_MADE_PROGRESS, &subreq->flags);
247249
__set_bit(NETFS_SREQ_IN_PROGRESS, &subreq->flags);
248250
netfs_do_issue_write(stream, subreq);
249251
}

‎fs/smb/client/cifssmb.c

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1319,14 +1319,16 @@ cifs_readv_callback(struct mid_q_entry *mid)
13191319
}
13201320

13211321
if (rdata->result == -ENODATA) {
1322-
__set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
13231322
rdata->result = 0;
1323+
__set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
13241324
} else {
13251325
size_t trans = rdata->subreq.transferred + rdata->got_bytes;
13261326
if (trans < rdata->subreq.len &&
13271327
rdata->subreq.start + trans == ictx->remote_i_size) {
1328-
__set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
13291328
rdata->result = 0;
1329+
__set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
1330+
} else if (rdata->got_bytes > 0) {
1331+
__set_bit(NETFS_SREQ_MADE_PROGRESS, &rdata->subreq.flags);
13301332
}
13311333
}
13321334

@@ -1670,10 +1672,13 @@ cifs_writev_callback(struct mid_q_entry *mid)
16701672
if (written > wdata->subreq.len)
16711673
written &= 0xFFFF;
16721674

1673-
if (written < wdata->subreq.len)
1675+
if (written < wdata->subreq.len) {
16741676
result = -ENOSPC;
1675-
else
1677+
} else {
16761678
result = written;
1679+
if (written > 0)
1680+
__set_bit(NETFS_SREQ_MADE_PROGRESS, &wdata->subreq.flags);
1681+
}
16771682
break;
16781683
case MID_REQUEST_SUBMITTED:
16791684
case MID_RETRY_NEEDED:

‎fs/smb/client/smb2pdu.c

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4615,6 +4615,7 @@ smb2_readv_callback(struct mid_q_entry *mid)
46154615
__set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
46164616
rdata->result = 0;
46174617
}
4618+
__set_bit(NETFS_SREQ_MADE_PROGRESS, &rdata->subreq.flags);
46184619
}
46194620
trace_smb3_rw_credits(rreq_debug_id, subreq_debug_index, rdata->credits.value,
46204621
server->credits, server->in_flight,
@@ -4840,10 +4841,12 @@ smb2_writev_callback(struct mid_q_entry *mid)
48404841
if (written > wdata->subreq.len)
48414842
written &= 0xFFFF;
48424843

4843-
if (written < wdata->subreq.len)
4844+
if (written < wdata->subreq.len) {
48444845
wdata->result = -ENOSPC;
4845-
else
4846+
} else if (written > 0) {
48464847
wdata->subreq.len = written;
4848+
__set_bit(NETFS_SREQ_MADE_PROGRESS, &wdata->subreq.flags);
4849+
}
48474850
break;
48484851
case MID_REQUEST_SUBMITTED:
48494852
case MID_RETRY_NEEDED:
@@ -5012,7 +5015,7 @@ smb2_async_writev(struct cifs_io_subrequest *wdata)
50125015
}
50135016
#endif
50145017

5015-
if (test_bit(NETFS_SREQ_RETRYING, &wdata->subreq.flags))
5018+
if (wdata->subreq.retry_count > 0)
50165019
smb2_set_replay(server, &rqst);
50175020

50185021
cifs_dbg(FYI, "async write at %llu %u bytes iter=%zx\n",

‎include/linux/netfs.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@ struct netfs_io_subrequest {
185185
short error; /* 0 or error that occurred */
186186
unsigned short debug_index; /* Index in list (for debugging output) */
187187
unsigned int nr_segs; /* Number of segs in io_iter */
188+
u8 retry_count; /* The number of retries (0 on initial pass) */
188189
enum netfs_io_source source; /* Where to read from/write to */
189190
unsigned char stream_nr; /* I/O stream this belongs to */
190191
unsigned char curr_folioq_slot; /* Folio currently being read */
@@ -194,14 +195,13 @@ struct netfs_io_subrequest {
194195
#define NETFS_SREQ_COPY_TO_CACHE 0 /* Set if should copy the data to the cache */
195196
#define NETFS_SREQ_CLEAR_TAIL 1 /* Set if the rest of the read should be cleared */
196197
#define NETFS_SREQ_SEEK_DATA_READ 3 /* Set if ->read() should SEEK_DATA first */
197-
#define NETFS_SREQ_NO_PROGRESS 4 /* Set if we didn't manage to read any data */
198+
#define NETFS_SREQ_MADE_PROGRESS 4 /* Set if we transferred at least some data */
198199
#define NETFS_SREQ_ONDEMAND 5 /* Set if it's from on-demand read mode */
199200
#define NETFS_SREQ_BOUNDARY 6 /* Set if ends on hard boundary (eg. ceph object) */
200201
#define NETFS_SREQ_HIT_EOF 7 /* Set if short due to EOF */
201202
#define NETFS_SREQ_IN_PROGRESS 8 /* Unlocked when the subrequest completes */
202203
#define NETFS_SREQ_NEED_RETRY 9 /* Set if the filesystem requests a retry */
203-
#define NETFS_SREQ_RETRYING 10 /* Set if we're retrying */
204-
#define NETFS_SREQ_FAILED 11 /* Set if the subreq failed unretryably */
204+
#define NETFS_SREQ_FAILED 10 /* Set if the subreq failed unretryably */
205205
};
206206

207207
enum netfs_io_origin {

0 commit comments

Comments
 (0)