Re: Kernel oops with 6.14 when enabling TLS

Re: Kernel oops with 6.14 when enabling TLS

[Vlastimil Babka]

+Cc NETWORKING [TLS] maintainers and netdev for input, thanks.

The full error is here:
https://lore.kernel.org/all/fcfa11c6-2738-4a2e-baa8-09fa8f79cbf3@suse.de/

On 3/4/25 11:20, Hannes Reinecke wrote:
> On 3/4/25 09:18, Vlastimil Babka wrote:
>> On 3/4/25 08:58, Hannes Reinecke wrote:
>>> On 3/3/25 23:02, Vlastimil Babka wrote:
>>>> On 3/3/25 17:15, Vlastimil Babka wrote:
>>>>> On 3/3/25 16:48, Matthew Wilcox wrote:
>>>>>> You need to turn on the debugging options Vlastimil mentioned and try to
>>>>>> figure out what nvme is doing wrong.
>>>>>
>>>>> Agree, looks like some error path going wrong?
>>>>> Since there seems to be actual non-large kmalloc usage involved, another
>>>>> debug parameter that could help: CONFIG_SLUB_DEBUG=y, and boot with
>>>>> "slab_debug=FZPU,kmalloc-*"
>>>>
>>>> Also make sure you have CONFIG_DEBUG_VM please.
>>>>
>>> Here you go:
>>>
>>> [  134.506802] page: refcount:0 mapcount:0 mapping:0000000000000000
>>> index:0x0 pfn:0x101ef8
>>> [  134.509253] head: order:3 mapcount:0 entire_mapcount:0
>>> nr_pages_mapped:0 pincount:0
>>> [  134.511594] flags:
>>> 0x17ffffc0000040(head|node=0|zone=2|lastcpupid=0x1fffff)
>>> [  134.513556] page_type: f5(slab)
>>> [  134.513563] raw: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
>>> ffff8881000402f0
>>> [  134.513568] raw: 0000000000000000 00000000000a000a 00000000f5000000
>>> 0000000000000000
>>> [  134.513572] head: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
>>> ffff8881000402f0
>>> [  134.513575] head: 0000000000000000 00000000000a000a 00000000f5000000
>>> 0000000000000000
>>> [  134.513579] head: 0017ffffc0000003 ffffea000407be01 ffffffffffffffff
>>> 0000000000000000
>>> [  134.513583] head: 0000000000000008 0000000000000000 00000000ffffffff
>>> 0000000000000000
>>> [  134.513585] page dumped because: VM_BUG_ON_FOLIO(((unsigned int)
>>> folio_ref_count(folio) + 127u <= 127u))
>>> [  134.513615] ------------[ cut here ]------------
>>> [  134.529822] kernel BUG at ./include/linux/mm.h:1455!
>> 
>> Yeah, just as I suspected, folio_get() says the refcount is 0.
>> 
>>> [  134.529835] Oops: invalid opcode: 0000 [#1] PREEMPT SMP
>>> DEBUG_PAGEALLOC NOPTI
>>> [  134.529843] CPU: 0 UID: 0 PID: 274 Comm: kworker/0:1H Kdump: loaded
>>> Tainted: G            E      6.14.0-rc4-default+ #309
>>> 03b131f1ef70944969b40df9d90a283ed638556f
>>> [  134.536577] Tainted: [E]=UNSIGNED_MODULE
>>> [  134.536580] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
>>> 0.0.0 02/06/2015
>>> [  134.536583] Workqueue: nvme_tcp_wq nvme_tcp_io_work [nvme_tcp]
>>> [  134.536595] RIP: 0010:__iov_iter_get_pages_alloc+0x676/0x710
>>> [  134.542810] Code: e8 4c 39 e0 49 0f 47 c4 48 01 45 08 48 29 45 18 e9
>>> 90 fa ff ff 48 83 ef 01 e9 7f fe ff ff 48 c7 c6 40 57 4f 82 e8 6a e2 ce
>>> ff <0f> 0b e8 43 b8 b1 ff eb c5 f7 c1 ff 0f 00 00 48 89 cf 0f 85 4f ff
>>> [  134.542816] RSP: 0018:ffffc900004579d8 EFLAGS: 00010282
>>> [  134.542821] RAX: 000000000000005c RBX: ffffc90000457a90 RCX:
>>> 0000000000000027
>>> [  134.542825] RDX: 0000000000000000 RSI: 0000000000000002 RDI:
>>> ffff88817f423748
>>> [  134.542828] RBP: ffffc90000457d60 R08: 0000000000000000 R09:
>>> 0000000000000001
>>> [  134.554485] R10: ffffc900004579c0 R11: ffffc90000457720 R12:
>>> 0000000000000000
>>> [  134.554488] R13: ffffea000407be40 R14: ffffc90000457a70 R15:
>>> ffffc90000457d60
>>> [  134.554495] FS:  0000000000000000(0000) GS:ffff88817f400000(0000)
>>> knlGS:0000000000000000
>>> [  134.554499] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [  134.554502] CR2: 0000556b0675b600 CR3: 0000000106bd8000 CR4:
>>> 0000000000350ef0
>>> [  134.554509] Call Trace:
>>> [  134.554512]  <TASK>
>>> [  134.554516]  ? __die_body+0x1a/0x60
>>> [  134.554525]  ? die+0x38/0x60
>>> [  134.554531]  ? do_trap+0x10f/0x120
>>> [  134.554538]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>> [  134.568839]  ? do_error_trap+0x64/0xa0
>>> [  134.568847]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>> [  134.568855]  ? exc_invalid_op+0x53/0x60
>>> [  134.572489]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>> [  134.572496]  ? asm_exc_invalid_op+0x16/0x20
>>> [  134.572512]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>> [  134.576726]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>> [  134.576733]  ? srso_return_thunk+0x5/0x5f
>>> [  134.576740]  ? ___slab_alloc+0x924/0xb60
>>> [  134.580253]  ? mempool_alloc_noprof+0x41/0x190
>>> [  134.580262]  ? tls_get_rec+0x3d/0x1b0 [tls
>>> 47f199c97f69357468c91efdbba24395e9dbfa77]
>>> [  134.580282]  iov_iter_get_pages2+0x19/0x30
>> 
>> Presumably that's __iov_iter_get_pages_alloc() doing get_page() either in
>> the " if (iov_iter_is_bvec(i)) " branch or via iter_folioq_get_pages()?
>> 
> Looks like it.
> 
>> Which doesn't work for a sub-size kmalloc() from a slab folio, which after
>> the frozen refcount conversion no longer supports get_page().
>> 
>> The question is if this is a mistake specific for this path that's easy to
>> fix or there are more paths that do this. At the very least the pinning of
>> page through a kmalloc() allocation from it is useless - the object itself
>> has to be kfree()'d and that would never happen through a put_page()
>> reaching zero.
>> 
> Looks like a specific mistake.
> tls_sw is the only user of sk_msg_zerocopy_from_iter()
> (which is calling into __iov_iter_get_pages_alloc()).
> 
> And, more to the point, tls_sw messes up iov pacing coming in from
> the upper layers.
> So even if the upper layers send individual iovs (where each iov might
> contain different allocation types), tls_sw is packing them together 
> into full records. So it might end up with iovs having _different_ 
> allocations.
> Which would explain why we only see it with TLS, but not with normal
> connections.
> 
> Or so my reasoning goes. Not sure if that's correct.
> 
> So I'd be happy with an 'easy' fix for now. Obviously :-)
> 
> Cheers,
> 
> Hannes

Re: Kernel oops with 6.14 when enabling TLS

[Hannes Reinecke]

On 3/4/25 11:26, Vlastimil Babka wrote:
> On 3/4/25 11:20, Hannes Reinecke wrote:

[ .. ]
>> So I'd be happy with an 'easy' fix for now. Obviously :-)
>>

With this patch:

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 65f550cb5081..b035a9928cdd 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1190,8 +1190,14 @@ static ssize_t __iov_iter_get_pages_alloc(struct 
iov_iter *i,
                 if (!n)
                         return -ENOMEM;
                 p = *pages;
-               for (int k = 0; k < n; k++)
-                       get_page(p[k] = page + k);
+               for (int k = 0; k < n; k++) {
+                       if (!get_page_unless_zero(p[k] = page + k)) {
+                               pr_warn("%s: frozen page %d of %d\n",
+                                       __func__, k, n);
+                               return -ENOMEM;
+                       }
+               }
+
                 maxsize = min_t(size_t, maxsize, n * PAGE_SIZE - *start);
                 i->count -= maxsize;
                 i->iov_offset += maxsize;


the system doesn't crash anymore:
[   51.520949] __iov_iter_get_pages_alloc: frozen page 0 of 1
[   51.536393] nvme nvme0: creating 4 I/O queues.
[   51.968897] nvme nvme0: mapped 4/0/0 default/read/poll queues.
[   51.972207] __iov_iter_get_pages_alloc: frozen page 0 of 1
[   51.974528] __iov_iter_get_pages_alloc: frozen page 0 of 1
[   51.976928] __iov_iter_get_pages_alloc: frozen page 0 of 1
[   51.978980] __iov_iter_get_pages_alloc: frozen page 0 of 1
[   51.981236] nvme nvme0: new ctrl: NQN "nqn.blktests-subsystem-1", 
addr 10.161.9.19:4420, hostnqn: 
nqn.2014-08.org.nvmexpress:uuid:027a49dc-b554-40e5-b0f9-0a9ea03ec30c

and the allocation in question is coming from
drivers/nvme/host/fabrics.c:nvmf_connect_data_prep(), which
coincidentally _is_ a kmalloc()ed buffer.

But TLS doesn't work, either:

[   58.886754] nvme nvme0: I/O tag 1 (3001) type 4 opcode 0x18 (Keep 
Alive) QID 0 timeout
[   58.889112] nvme nvme0: starting error recovery
[   58.892176] nvme nvme0: failed nvme_keep_alive_end_io error=10
[   58.892282] nvme nvme0: reading non-mdts-limits failed: -4
[   58.902490] nvme nvme0: Reconnecting in 10 seconds...

(probably not surprising seeing that an error is returned ..)

So yeah, looks like TLS has issues with kmalloced data.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: Kernel oops with 6.14 when enabling TLS

[Vlastimil Babka]

On 3/4/25 11:26, Vlastimil Babka wrote:
> +Cc NETWORKING [TLS] maintainers and netdev for input, thanks.
> 
> The full error is here:
> https://lore.kernel.org/all/fcfa11c6-2738-4a2e-baa8-09fa8f79cbf3@suse.de/
> 
> On 3/4/25 11:20, Hannes Reinecke wrote:
>> On 3/4/25 09:18, Vlastimil Babka wrote:
>>> On 3/4/25 08:58, Hannes Reinecke wrote:
>>>> On 3/3/25 23:02, Vlastimil Babka wrote:
>>>>> On 3/3/25 17:15, Vlastimil Babka wrote:
>>>>>> On 3/3/25 16:48, Matthew Wilcox wrote:
>>>>>>> You need to turn on the debugging options Vlastimil mentioned and try to
>>>>>>> figure out what nvme is doing wrong.
>>>>>>
>>>>>> Agree, looks like some error path going wrong?
>>>>>> Since there seems to be actual non-large kmalloc usage involved, another
>>>>>> debug parameter that could help: CONFIG_SLUB_DEBUG=y, and boot with
>>>>>> "slab_debug=FZPU,kmalloc-*"
>>>>>
>>>>> Also make sure you have CONFIG_DEBUG_VM please.
>>>>>
>>>> Here you go:
>>>>
>>>> [  134.506802] page: refcount:0 mapcount:0 mapping:0000000000000000
>>>> index:0x0 pfn:0x101ef8
>>>> [  134.509253] head: order:3 mapcount:0 entire_mapcount:0
>>>> nr_pages_mapped:0 pincount:0
>>>> [  134.511594] flags:
>>>> 0x17ffffc0000040(head|node=0|zone=2|lastcpupid=0x1fffff)
>>>> [  134.513556] page_type: f5(slab)
>>>> [  134.513563] raw: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
>>>> ffff8881000402f0
>>>> [  134.513568] raw: 0000000000000000 00000000000a000a 00000000f5000000
>>>> 0000000000000000
>>>> [  134.513572] head: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
>>>> ffff8881000402f0
>>>> [  134.513575] head: 0000000000000000 00000000000a000a 00000000f5000000
>>>> 0000000000000000
>>>> [  134.513579] head: 0017ffffc0000003 ffffea000407be01 ffffffffffffffff
>>>> 0000000000000000
>>>> [  134.513583] head: 0000000000000008 0000000000000000 00000000ffffffff
>>>> 0000000000000000
>>>> [  134.513585] page dumped because: VM_BUG_ON_FOLIO(((unsigned int)
>>>> folio_ref_count(folio) + 127u <= 127u))
>>>> [  134.513615] ------------[ cut here ]------------
>>>> [  134.529822] kernel BUG at ./include/linux/mm.h:1455!
>>> 
>>> Yeah, just as I suspected, folio_get() says the refcount is 0.
>>> 
>>>> [  134.529835] Oops: invalid opcode: 0000 [#1] PREEMPT SMP
>>>> DEBUG_PAGEALLOC NOPTI
>>>> [  134.529843] CPU: 0 UID: 0 PID: 274 Comm: kworker/0:1H Kdump: loaded
>>>> Tainted: G            E      6.14.0-rc4-default+ #309
>>>> 03b131f1ef70944969b40df9d90a283ed638556f
>>>> [  134.536577] Tainted: [E]=UNSIGNED_MODULE
>>>> [  134.536580] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
>>>> 0.0.0 02/06/2015
>>>> [  134.536583] Workqueue: nvme_tcp_wq nvme_tcp_io_work [nvme_tcp]
>>>> [  134.536595] RIP: 0010:__iov_iter_get_pages_alloc+0x676/0x710
>>>> [  134.542810] Code: e8 4c 39 e0 49 0f 47 c4 48 01 45 08 48 29 45 18 e9
>>>> 90 fa ff ff 48 83 ef 01 e9 7f fe ff ff 48 c7 c6 40 57 4f 82 e8 6a e2 ce
>>>> ff <0f> 0b e8 43 b8 b1 ff eb c5 f7 c1 ff 0f 00 00 48 89 cf 0f 85 4f ff
>>>> [  134.542816] RSP: 0018:ffffc900004579d8 EFLAGS: 00010282
>>>> [  134.542821] RAX: 000000000000005c RBX: ffffc90000457a90 RCX:
>>>> 0000000000000027
>>>> [  134.542825] RDX: 0000000000000000 RSI: 0000000000000002 RDI:
>>>> ffff88817f423748
>>>> [  134.542828] RBP: ffffc90000457d60 R08: 0000000000000000 R09:
>>>> 0000000000000001
>>>> [  134.554485] R10: ffffc900004579c0 R11: ffffc90000457720 R12:
>>>> 0000000000000000
>>>> [  134.554488] R13: ffffea000407be40 R14: ffffc90000457a70 R15:
>>>> ffffc90000457d60
>>>> [  134.554495] FS:  0000000000000000(0000) GS:ffff88817f400000(0000)
>>>> knlGS:0000000000000000
>>>> [  134.554499] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [  134.554502] CR2: 0000556b0675b600 CR3: 0000000106bd8000 CR4:
>>>> 0000000000350ef0
>>>> [  134.554509] Call Trace:
>>>> [  134.554512]  <TASK>
>>>> [  134.554516]  ? __die_body+0x1a/0x60
>>>> [  134.554525]  ? die+0x38/0x60
>>>> [  134.554531]  ? do_trap+0x10f/0x120
>>>> [  134.554538]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>> [  134.568839]  ? do_error_trap+0x64/0xa0
>>>> [  134.568847]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>> [  134.568855]  ? exc_invalid_op+0x53/0x60
>>>> [  134.572489]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>> [  134.572496]  ? asm_exc_invalid_op+0x16/0x20
>>>> [  134.572512]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>> [  134.576726]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>> [  134.576733]  ? srso_return_thunk+0x5/0x5f
>>>> [  134.576740]  ? ___slab_alloc+0x924/0xb60
>>>> [  134.580253]  ? mempool_alloc_noprof+0x41/0x190
>>>> [  134.580262]  ? tls_get_rec+0x3d/0x1b0 [tls
>>>> 47f199c97f69357468c91efdbba24395e9dbfa77]
>>>> [  134.580282]  iov_iter_get_pages2+0x19/0x30
>>> 
>>> Presumably that's __iov_iter_get_pages_alloc() doing get_page() either in
>>> the " if (iov_iter_is_bvec(i)) " branch or via iter_folioq_get_pages()?
>>> 
>> Looks like it.
>> 
>>> Which doesn't work for a sub-size kmalloc() from a slab folio, which after
>>> the frozen refcount conversion no longer supports get_page().
>>> 
>>> The question is if this is a mistake specific for this path that's easy to
>>> fix or there are more paths that do this. At the very least the pinning of
>>> page through a kmalloc() allocation from it is useless - the object itself
>>> has to be kfree()'d and that would never happen through a put_page()
>>> reaching zero.
>>> 
>> Looks like a specific mistake.
>> tls_sw is the only user of sk_msg_zerocopy_from_iter()
>> (which is calling into __iov_iter_get_pages_alloc()).

That's from tls_sw_sendmsg_locked(), right? But that's under:

if (!is_kvec && (full_record || eor) && !async_capable) {

Shouldn't is_kvec be true if we're dealing a kernel buffer (kmalloc()) there?

>> And, more to the point, tls_sw messes up iov pacing coming in from
>> the upper layers.
>> So even if the upper layers send individual iovs (where each iov might
>> contain different allocation types), tls_sw is packing them together 
>> into full records. So it might end up with iovs having _different_ 
>> allocations.
>> Which would explain why we only see it with TLS, but not with normal
>> connections.
>> 
>> Or so my reasoning goes. Not sure if that's correct.
>> 
>> So I'd be happy with an 'easy' fix for now. Obviously :-)
>> 
>> Cheers,
>> 
>> Hannes
> 

Re: Kernel oops with 6.14 when enabling TLS

[Hannes Reinecke]

On 3/4/25 16:29, Vlastimil Babka wrote:
> On 3/4/25 11:26, Vlastimil Babka wrote:
>> +Cc NETWORKING [TLS] maintainers and netdev for input, thanks.
>>
>> The full error is here:
>> https://lore.kernel.org/all/fcfa11c6-2738-4a2e-baa8-09fa8f79cbf3@suse.de/
>>
>> On 3/4/25 11:20, Hannes Reinecke wrote:
>>> On 3/4/25 09:18, Vlastimil Babka wrote:
>>>> On 3/4/25 08:58, Hannes Reinecke wrote:
>>>>> On 3/3/25 23:02, Vlastimil Babka wrote:
>>>>>> On 3/3/25 17:15, Vlastimil Babka wrote:
>>>>>>> On 3/3/25 16:48, Matthew Wilcox wrote:
>>>>>>>> You need to turn on the debugging options Vlastimil mentioned and try to
>>>>>>>> figure out what nvme is doing wrong.
>>>>>>>
>>>>>>> Agree, looks like some error path going wrong?
>>>>>>> Since there seems to be actual non-large kmalloc usage involved, another
>>>>>>> debug parameter that could help: CONFIG_SLUB_DEBUG=y, and boot with
>>>>>>> "slab_debug=FZPU,kmalloc-*"
>>>>>>
>>>>>> Also make sure you have CONFIG_DEBUG_VM please.
>>>>>>
>>>>> Here you go:
>>>>>
>>>>> [  134.506802] page: refcount:0 mapcount:0 mapping:0000000000000000
>>>>> index:0x0 pfn:0x101ef8
>>>>> [  134.509253] head: order:3 mapcount:0 entire_mapcount:0
>>>>> nr_pages_mapped:0 pincount:0
>>>>> [  134.511594] flags:
>>>>> 0x17ffffc0000040(head|node=0|zone=2|lastcpupid=0x1fffff)
>>>>> [  134.513556] page_type: f5(slab)
>>>>> [  134.513563] raw: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
>>>>> ffff8881000402f0
>>>>> [  134.513568] raw: 0000000000000000 00000000000a000a 00000000f5000000
>>>>> 0000000000000000
>>>>> [  134.513572] head: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
>>>>> ffff8881000402f0
>>>>> [  134.513575] head: 0000000000000000 00000000000a000a 00000000f5000000
>>>>> 0000000000000000
>>>>> [  134.513579] head: 0017ffffc0000003 ffffea000407be01 ffffffffffffffff
>>>>> 0000000000000000
>>>>> [  134.513583] head: 0000000000000008 0000000000000000 00000000ffffffff
>>>>> 0000000000000000
>>>>> [  134.513585] page dumped because: VM_BUG_ON_FOLIO(((unsigned int)
>>>>> folio_ref_count(folio) + 127u <= 127u))
>>>>> [  134.513615] ------------[ cut here ]------------
>>>>> [  134.529822] kernel BUG at ./include/linux/mm.h:1455!
>>>>
>>>> Yeah, just as I suspected, folio_get() says the refcount is 0.
>>>>
>>>>> [  134.529835] Oops: invalid opcode: 0000 [#1] PREEMPT SMP
>>>>> DEBUG_PAGEALLOC NOPTI
>>>>> [  134.529843] CPU: 0 UID: 0 PID: 274 Comm: kworker/0:1H Kdump: loaded
>>>>> Tainted: G            E      6.14.0-rc4-default+ #309
>>>>> 03b131f1ef70944969b40df9d90a283ed638556f
>>>>> [  134.536577] Tainted: [E]=UNSIGNED_MODULE
>>>>> [  134.536580] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
>>>>> 0.0.0 02/06/2015
>>>>> [  134.536583] Workqueue: nvme_tcp_wq nvme_tcp_io_work [nvme_tcp]
>>>>> [  134.536595] RIP: 0010:__iov_iter_get_pages_alloc+0x676/0x710
>>>>> [  134.542810] Code: e8 4c 39 e0 49 0f 47 c4 48 01 45 08 48 29 45 18 e9
>>>>> 90 fa ff ff 48 83 ef 01 e9 7f fe ff ff 48 c7 c6 40 57 4f 82 e8 6a e2 ce
>>>>> ff <0f> 0b e8 43 b8 b1 ff eb c5 f7 c1 ff 0f 00 00 48 89 cf 0f 85 4f ff
>>>>> [  134.542816] RSP: 0018:ffffc900004579d8 EFLAGS: 00010282
>>>>> [  134.542821] RAX: 000000000000005c RBX: ffffc90000457a90 RCX:
>>>>> 0000000000000027
>>>>> [  134.542825] RDX: 0000000000000000 RSI: 0000000000000002 RDI:
>>>>> ffff88817f423748
>>>>> [  134.542828] RBP: ffffc90000457d60 R08: 0000000000000000 R09:
>>>>> 0000000000000001
>>>>> [  134.554485] R10: ffffc900004579c0 R11: ffffc90000457720 R12:
>>>>> 0000000000000000
>>>>> [  134.554488] R13: ffffea000407be40 R14: ffffc90000457a70 R15:
>>>>> ffffc90000457d60
>>>>> [  134.554495] FS:  0000000000000000(0000) GS:ffff88817f400000(0000)
>>>>> knlGS:0000000000000000
>>>>> [  134.554499] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> [  134.554502] CR2: 0000556b0675b600 CR3: 0000000106bd8000 CR4:
>>>>> 0000000000350ef0
>>>>> [  134.554509] Call Trace:
>>>>> [  134.554512]  <TASK>
>>>>> [  134.554516]  ? __die_body+0x1a/0x60
>>>>> [  134.554525]  ? die+0x38/0x60
>>>>> [  134.554531]  ? do_trap+0x10f/0x120
>>>>> [  134.554538]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>>> [  134.568839]  ? do_error_trap+0x64/0xa0
>>>>> [  134.568847]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>>> [  134.568855]  ? exc_invalid_op+0x53/0x60
>>>>> [  134.572489]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>>> [  134.572496]  ? asm_exc_invalid_op+0x16/0x20
>>>>> [  134.572512]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>>> [  134.576726]  ? __iov_iter_get_pages_alloc+0x676/0x710
>>>>> [  134.576733]  ? srso_return_thunk+0x5/0x5f
>>>>> [  134.576740]  ? ___slab_alloc+0x924/0xb60
>>>>> [  134.580253]  ? mempool_alloc_noprof+0x41/0x190
>>>>> [  134.580262]  ? tls_get_rec+0x3d/0x1b0 [tls
>>>>> 47f199c97f69357468c91efdbba24395e9dbfa77]
>>>>> [  134.580282]  iov_iter_get_pages2+0x19/0x30
>>>>
>>>> Presumably that's __iov_iter_get_pages_alloc() doing get_page() either in
>>>> the " if (iov_iter_is_bvec(i)) " branch or via iter_folioq_get_pages()?
>>>>
>>> Looks like it.
>>>
>>>> Which doesn't work for a sub-size kmalloc() from a slab folio, which after
>>>> the frozen refcount conversion no longer supports get_page().
>>>>
>>>> The question is if this is a mistake specific for this path that's easy to
>>>> fix or there are more paths that do this. At the very least the pinning of
>>>> page through a kmalloc() allocation from it is useless - the object itself
>>>> has to be kfree()'d and that would never happen through a put_page()
>>>> reaching zero.
>>>>
>>> Looks like a specific mistake.
>>> tls_sw is the only user of sk_msg_zerocopy_from_iter()
>>> (which is calling into __iov_iter_get_pages_alloc()).
> 
> That's from tls_sw_sendmsg_locked(), right? But that's under:
> 
> if (!is_kvec && (full_record || eor) && !async_capable) {
> 
> Shouldn't is_kvec be true if we're dealing a kernel buffer (kmalloc()) there?
> 
Yes, and no.

We're initializing the iter in nvme_tcp_try_send_data():

		bvec_set_page(&bvec, page, len, offset);
		iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, &bvec, 1, len);

and 'page' is coming from bio bvec. So the bv_page could refer to a 
kmalloced page, the bvec is still that, a bvec.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: Kernel oops with 6.14 when enabling TLS

[Matthew Wilcox]

On Tue, Mar 04, 2025 at 11:26:07AM +0100, Vlastimil Babka wrote:
> +Cc NETWORKING [TLS] maintainers and netdev for input, thanks.
> 
> The full error is here:
> https://lore.kernel.org/all/fcfa11c6-2738-4a2e-baa8-09fa8f79cbf3@suse.de/
> 
> On 3/4/25 11:20, Hannes Reinecke wrote:
> > On 3/4/25 09:18, Vlastimil Babka wrote:
> >> On 3/4/25 08:58, Hannes Reinecke wrote:
> >>> On 3/3/25 23:02, Vlastimil Babka wrote:
> >>>> Also make sure you have CONFIG_DEBUG_VM please.
> >>>>
> >>> Here you go:
> >>>
> >>> [  134.506802] page: refcount:0 mapcount:0 mapping:0000000000000000
> >>> index:0x0 pfn:0x101ef8
> >>> [  134.509253] head: order:3 mapcount:0 entire_mapcount:0
> >>> nr_pages_mapped:0 pincount:0
> >>> [  134.511594] flags:
> >>> 0x17ffffc0000040(head|node=0|zone=2|lastcpupid=0x1fffff)
> >>> [  134.513556] page_type: f5(slab)
> >>> [  134.513563] raw: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
> >>> ffff8881000402f0
> >>> [  134.513568] raw: 0000000000000000 00000000000a000a 00000000f5000000
> >>> 0000000000000000
> >>> [  134.513572] head: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
> >>> ffff8881000402f0
> >>> [  134.513575] head: 0000000000000000 00000000000a000a 00000000f5000000
> >>> 0000000000000000
> >>> [  134.513579] head: 0017ffffc0000003 ffffea000407be01 ffffffffffffffff
> >>> 0000000000000000
> >>> [  134.513583] head: 0000000000000008 0000000000000000 00000000ffffffff
> >>> 0000000000000000
> >>> [  134.513585] page dumped because: VM_BUG_ON_FOLIO(((unsigned int)
> >>> folio_ref_count(folio) + 127u <= 127u))
> >>> [  134.513615] ------------[ cut here ]------------
> >>> [  134.529822] kernel BUG at ./include/linux/mm.h:1455!
> >> 
> >> Yeah, just as I suspected, folio_get() says the refcount is 0.

... and it has a page_type of f5 (slab)

> >>> [  134.554509] Call Trace:
> >>> [  134.580282]  iov_iter_get_pages2+0x19/0x30
> >> 
> >> Presumably that's __iov_iter_get_pages_alloc() doing get_page() either in
> >> the " if (iov_iter_is_bvec(i)) " branch or via iter_folioq_get_pages()?

It's the bvec path:

                iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, &bvec, 1, len);

> >> Which doesn't work for a sub-size kmalloc() from a slab folio, which after
> >> the frozen refcount conversion no longer supports get_page().
> >> 
> >> The question is if this is a mistake specific for this path that's easy to
> >> fix or there are more paths that do this. At the very least the pinning of
> >> page through a kmalloc() allocation from it is useless - the object itself
> >> has to be kfree()'d and that would never happen through a put_page()
> >> reaching zero.
> >> 
> > Looks like a specific mistake.
> > tls_sw is the only user of sk_msg_zerocopy_from_iter()
> > (which is calling into __iov_iter_get_pages_alloc()).
> > 
> > And, more to the point, tls_sw messes up iov pacing coming in from
> > the upper layers.
> > So even if the upper layers send individual iovs (where each iov might
> > contain different allocation types), tls_sw is packing them together 
> > into full records. So it might end up with iovs having _different_ 
> > allocations.
> > Which would explain why we only see it with TLS, but not with normal
> > connections.

I thought we'd done all the work needed to get rid of these pointless
refcount bumps.  Turns out that's only on the block side (eg commit
e4cc64657bec).  So what does networking need in order to understand
that some iovecs do not need to mess with the refcount?

Re: Kernel oops with 6.14 when enabling TLS

[Hannes Reinecke]

On 3/4/25 17:14, Matthew Wilcox wrote:
> On Tue, Mar 04, 2025 at 11:26:07AM +0100, Vlastimil Babka wrote:
>> +Cc NETWORKING [TLS] maintainers and netdev for input, thanks.
>>
>> The full error is here:
>> https://lore.kernel.org/all/fcfa11c6-2738-4a2e-baa8-09fa8f79cbf3@suse.de/
>>
>> On 3/4/25 11:20, Hannes Reinecke wrote:
>>> On 3/4/25 09:18, Vlastimil Babka wrote:
>>>> On 3/4/25 08:58, Hannes Reinecke wrote:
>>>>> On 3/3/25 23:02, Vlastimil Babka wrote:
>>>>>> Also make sure you have CONFIG_DEBUG_VM please.
>>>>>>
>>>>> Here you go:
>>>>>
>>>>> [  134.506802] page: refcount:0 mapcount:0 mapping:0000000000000000
>>>>> index:0x0 pfn:0x101ef8
>>>>> [  134.509253] head: order:3 mapcount:0 entire_mapcount:0
>>>>> nr_pages_mapped:0 pincount:0
>>>>> [  134.511594] flags:
>>>>> 0x17ffffc0000040(head|node=0|zone=2|lastcpupid=0x1fffff)
>>>>> [  134.513556] page_type: f5(slab)
>>>>> [  134.513563] raw: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
>>>>> ffff8881000402f0
>>>>> [  134.513568] raw: 0000000000000000 00000000000a000a 00000000f5000000
>>>>> 0000000000000000
>>>>> [  134.513572] head: 0017ffffc0000040 ffff888100041b00 ffffea0004a90810
>>>>> ffff8881000402f0
>>>>> [  134.513575] head: 0000000000000000 00000000000a000a 00000000f5000000
>>>>> 0000000000000000
>>>>> [  134.513579] head: 0017ffffc0000003 ffffea000407be01 ffffffffffffffff
>>>>> 0000000000000000
>>>>> [  134.513583] head: 0000000000000008 0000000000000000 00000000ffffffff
>>>>> 0000000000000000
>>>>> [  134.513585] page dumped because: VM_BUG_ON_FOLIO(((unsigned int)
>>>>> folio_ref_count(folio) + 127u <= 127u))
>>>>> [  134.513615] ------------[ cut here ]------------
>>>>> [  134.529822] kernel BUG at ./include/linux/mm.h:1455!
>>>>
>>>> Yeah, just as I suspected, folio_get() says the refcount is 0.
> 
> ... and it has a page_type of f5 (slab)
> 
>>>>> [  134.554509] Call Trace:
>>>>> [  134.580282]  iov_iter_get_pages2+0x19/0x30
>>>>
>>>> Presumably that's __iov_iter_get_pages_alloc() doing get_page() either in
>>>> the " if (iov_iter_is_bvec(i)) " branch or via iter_folioq_get_pages()?
> 
> It's the bvec path:
> 
>                  iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, &bvec, 1, len);
> 
>>>> Which doesn't work for a sub-size kmalloc() from a slab folio, which after
>>>> the frozen refcount conversion no longer supports get_page().
>>>>
>>>> The question is if this is a mistake specific for this path that's easy to
>>>> fix or there are more paths that do this. At the very least the pinning of
>>>> page through a kmalloc() allocation from it is useless - the object itself
>>>> has to be kfree()'d and that would never happen through a put_page()
>>>> reaching zero.
>>>>
>>> Looks like a specific mistake.
>>> tls_sw is the only user of sk_msg_zerocopy_from_iter()
>>> (which is calling into __iov_iter_get_pages_alloc()).
>>>
>>> And, more to the point, tls_sw messes up iov pacing coming in from
>>> the upper layers.
>>> So even if the upper layers send individual iovs (where each iov might
>>> contain different allocation types), tls_sw is packing them together
>>> into full records. So it might end up with iovs having _different_
>>> allocations.
>>> Which would explain why we only see it with TLS, but not with normal
>>> connections.
> 
> I thought we'd done all the work needed to get rid of these pointless
> refcount bumps.  Turns out that's only on the block side (eg commit
> e4cc64657bec).  So what does networking need in order to understand
> that some iovecs do not need to mess with the refcount?

The network stack needs to get hold of the page while transmission is 
ongoing, as there is potentially rather deep queueing involved,
requiring several calls to sendmsg() and friends before the page is 
finally transmitted. And maybe some post-processing (checksums,
digests, you name it), too, all of which require the page to be there.

It's all so jumbled up ... personally, I would _love_ to do away with
__iov_iter_get_pages_alloc(). Allocating a page array? Seriously?

And the problem with that is that it's always takes a page(!) reference,
completely oblivious to the fact whether you even _can_ take a page 
reference (eg for tail pages); we've hit this problem several times now
(check for sendpage_ok() ...).

But that's not the real issue; real issue is that the page reference is
taken down in the very bowels of __iov_iter_get_pages_alloc(), but needs
to be undone by the _caller_. Who might (or might not) have an idea
that he needs to drop the reference here.
That's why there is no straightforward conversion; you need to audit
each and every caller and try to find out where the page reference (if 
any) is dropped.
Bah.

Can't we (at the very least) leave it to the caller of 
__iov_iter_get_pages() to get a page reference (he has access to the 
page array, after all ...)? That would make the interface slightly
better, and it'll be far more obvious to the caller what needs
to be done.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.com                               +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: Kernel oops with 6.14 when enabling TLS

[Matthew Wilcox]

On Tue, Mar 04, 2025 at 05:32:32PM +0100, Hannes Reinecke wrote:
> On 3/4/25 17:14, Matthew Wilcox wrote:
> > I thought we'd done all the work needed to get rid of these pointless
> > refcount bumps.  Turns out that's only on the block side (eg commit
> > e4cc64657bec).  So what does networking need in order to understand
> > that some iovecs do not need to mess with the refcount?
> 
> The network stack needs to get hold of the page while transmission is
> ongoing, as there is potentially rather deep queueing involved,
> requiring several calls to sendmsg() and friends before the page is finally
> transmitted. And maybe some post-processing (checksums,
> digests, you name it), too, all of which require the page to be there.
> 
> It's all so jumbled up ... personally, I would _love_ to do away with
> __iov_iter_get_pages_alloc(). Allocating a page array? Seriously?
> 
> And the problem with that is that it's always takes a page(!) reference,
> completely oblivious to the fact whether you even _can_ take a page
> reference (eg for tail pages); we've hit this problem several times now
> (check for sendpage_ok() ...).

Calling get_page() / put_page() on a tail page is fine -- that just
redirects to the head page.  But calling it on a slab never made any
sense; at best it gets you the equivalent of TYPESAFE_BY_RCU -- that is,
the object can be freed and reallocated, but the underlying slab will
not be reallocated to some other purpose.

> But that's not the real issue; real issue is that the page reference is
> taken down in the very bowels of __iov_iter_get_pages_alloc(), but needs
> to be undone by the _caller_. Who might (or might not) have an idea
> that he needs to drop the reference here.
> That's why there is no straightforward conversion; you need to audit
> each and every caller and try to find out where the page reference (if any)
> is dropped.
> Bah.
> 
> Can't we (at the very least) leave it to the caller of
> __iov_iter_get_pages() to get a page reference (he has access to the page
> array, after all ...)? That would make the interface slightly
> better, and it'll be far more obvious to the caller what needs
> to be done.

Right, that's what happened in the block layer.  We mark the bio with
BIO_PAGE_PINNED if the pincount needs to be dropped.  As a transitional
period, we had BIO_PAGE_REFFED which indicated that the page refcount
needed to be dropped.  Perhaps there's something similar that network
could be doing.

Re: Kernel oops with 6.14 when enabling TLS

[Matthew Wilcox]

On Tue, Mar 04, 2025 at 04:53:09PM +0000, Matthew Wilcox wrote:
> Right, that's what happened in the block layer.  We mark the bio with
> BIO_PAGE_PINNED if the pincount needs to be dropped.  As a transitional
> period, we had BIO_PAGE_REFFED which indicated that the page refcount
> needed to be dropped.  Perhaps there's something similar that network
> could be doing.

Until that time ... how does this look as a quick hack to avoid
reverting the slab change?

diff --git a/include/linux/mm.h b/include/linux/mm.h
index d6fed25243c3..ca08a923ac6d 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1520,7 +1520,10 @@ static inline void folio_get(struct folio *folio)
 
 static inline void get_page(struct page *page)
 {
-	folio_get(page_folio(page));
+	struct folio *folio = page_folio(page);
+	if (WARN_ON_ONCE(folio_test_slab(folio)))
+		return;
+	folio_get(folio);
 }
 
 static inline __must_check bool try_get_page(struct page *page)
@@ -1614,6 +1617,8 @@ static inline void put_page(struct page *page)
 {
 	struct folio *folio = page_folio(page);
 
+	if (folio_test_slab(folio))
+		return;
 	folio_put(folio);
 }
 
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 65f550cb5081..8c7fdb7d8c8f 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1190,8 +1190,12 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
 		if (!n)
 			return -ENOMEM;
 		p = *pages;
-		for (int k = 0; k < n; k++)
-			get_page(p[k] = page + k);
+		for (int k = 0; k < n; k++) {
+			struct folio *folio = page_folio(page);
+			p[k] = page + k;
+			if (!folio_test_slab(folio))
+				folio_get(folio);
+		}
 		maxsize = min_t(size_t, maxsize, n * PAGE_SIZE - *start);
 		i->count -= maxsize;
 		i->iov_offset += maxsize;

Re: Kernel oops with 6.14 when enabling TLS

[Vlastimil Babka]

On 3/4/25 19:05, Matthew Wilcox wrote:
> On Tue, Mar 04, 2025 at 04:53:09PM +0000, Matthew Wilcox wrote:
>> Right, that's what happened in the block layer.  We mark the bio with
>> BIO_PAGE_PINNED if the pincount needs to be dropped.  As a transitional
>> period, we had BIO_PAGE_REFFED which indicated that the page refcount
>> needed to be dropped.  Perhaps there's something similar that network
>> could be doing.
> 
> Until that time ... how does this look as a quick hack to avoid
> reverting the slab change?
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index d6fed25243c3..ca08a923ac6d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1520,7 +1520,10 @@ static inline void folio_get(struct folio *folio)
>  
>  static inline void get_page(struct page *page)
>  {
> -	folio_get(page_folio(page));
> +	struct folio *folio = page_folio(page);
> +	if (WARN_ON_ONCE(folio_test_slab(folio)))
> +		return;
> +	folio_get(folio);

Maybe we could do just VM_WARN_ON_ONCE if we fix the known offender fully.

>  }
>  
>  static inline __must_check bool try_get_page(struct page *page)
> @@ -1614,6 +1617,8 @@ static inline void put_page(struct page *page)
>  {
>  	struct folio *folio = page_folio(page);
>  
> +	if (folio_test_slab(folio))
> +		return;

And that means knowing where the put_page() side is and skipping it for slab
pages like for the get_page() side below. Hannes could you apply this patch
but also with WARN_ON_ONCE here in put_page()? And tell us where the
put_page() splats. Thanks!

>  	folio_put(folio);
>  }
>  
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 65f550cb5081..8c7fdb7d8c8f 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1190,8 +1190,12 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
>  		if (!n)
>  			return -ENOMEM;
>  		p = *pages;
> -		for (int k = 0; k < n; k++)
> -			get_page(p[k] = page + k);
> +		for (int k = 0; k < n; k++) {
> +			struct folio *folio = page_folio(page);
> +			p[k] = page + k;
> +			if (!folio_test_slab(folio))
> +				folio_get(folio);
> +		}
>  		maxsize = min_t(size_t, maxsize, n * PAGE_SIZE - *start);
>  		i->count -= maxsize;
>  		i->iov_offset += maxsize;

Re: Kernel oops with 6.14 when enabling TLS

[Hannes Reinecke]

On 3/4/25 19:05, Matthew Wilcox wrote:
> On Tue, Mar 04, 2025 at 04:53:09PM +0000, Matthew Wilcox wrote:
>> Right, that's what happened in the block layer.  We mark the bio with
>> BIO_PAGE_PINNED if the pincount needs to be dropped.  As a transitional
>> period, we had BIO_PAGE_REFFED which indicated that the page refcount
>> needed to be dropped.  Perhaps there's something similar that network
>> could be doing.
> 
> Until that time ... how does this look as a quick hack to avoid
> reverting the slab change?
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index d6fed25243c3..ca08a923ac6d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1520,7 +1520,10 @@ static inline void folio_get(struct folio *folio)
>   
>   static inline void get_page(struct page *page)
>   {
> -	folio_get(page_folio(page));
> +	struct folio *folio = page_folio(page);
> +	if (WARN_ON_ONCE(folio_test_slab(folio)))
> +		return;
> +	folio_get(folio);
>   }
>   
>   static inline __must_check bool try_get_page(struct page *page)
> @@ -1614,6 +1617,8 @@ static inline void put_page(struct page *page)
>   {
>   	struct folio *folio = page_folio(page);
>   
> +	if (folio_test_slab(folio))
> +		return;
>   	folio_put(folio);
>   }
>   
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 65f550cb5081..8c7fdb7d8c8f 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -1190,8 +1190,12 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
>   		if (!n)
>   			return -ENOMEM;
>   		p = *pages;
> -		for (int k = 0; k < n; k++)
> -			get_page(p[k] = page + k);
> +		for (int k = 0; k < n; k++) {
> +			struct folio *folio = page_folio(page);
> +			p[k] = page + k;
> +			if (!folio_test_slab(folio))
> +				folio_get(folio);
> +		}
>   		maxsize = min_t(size_t, maxsize, n * PAGE_SIZE - *start);
>   		i->count -= maxsize;
>   		i->iov_offset += maxsize;
> 

Good news and bad news ...
Good news: TLS works again!
Bad news: no errors.

Question to the wise: this is not the only place in iov_iter.c where we 
do a 'get_page()'. Do we leave them and wait for others to report 
regressions, knowing fully well that the current code _has_ issues?
Or shouldn't we rather clean them up?

I guess the real fix would be to fiddle with the 'bio_add_page()' logic;
we are always adding a 'page' reference to the bio, completely ignoring
whether this page is a slab page or a normal one.

Discussion at LSF, maybe?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: Kernel oops with 6.14 when enabling TLS

[Vlastimil Babka]

On 3/4/25 20:39, Hannes Reinecke wrote:
> On 3/4/25 19:05, Matthew Wilcox wrote:
>> On Tue, Mar 04, 2025 at 04:53:09PM +0000, Matthew Wilcox wrote:
>>> Right, that's what happened in the block layer.  We mark the bio with
>>> BIO_PAGE_PINNED if the pincount needs to be dropped.  As a transitional
>>> period, we had BIO_PAGE_REFFED which indicated that the page refcount
>>> needed to be dropped.  Perhaps there's something similar that network
>>> could be doing.
>> 
>> Until that time ... how does this look as a quick hack to avoid
>> reverting the slab change?
>> 
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index d6fed25243c3..ca08a923ac6d 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1520,7 +1520,10 @@ static inline void folio_get(struct folio *folio)
>>   
>>   static inline void get_page(struct page *page)
>>   {
>> -	folio_get(page_folio(page));
>> +	struct folio *folio = page_folio(page);
>> +	if (WARN_ON_ONCE(folio_test_slab(folio)))
>> +		return;
>> +	folio_get(folio);
>>   }
>>   
>>   static inline __must_check bool try_get_page(struct page *page)
>> @@ -1614,6 +1617,8 @@ static inline void put_page(struct page *page)
>>   {
>>   	struct folio *folio = page_folio(page);
>>   
>> +	if (folio_test_slab(folio))
>> +		return;
>>   	folio_put(folio);
>>   }
>>   
>> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
>> index 65f550cb5081..8c7fdb7d8c8f 100644
>> --- a/lib/iov_iter.c
>> +++ b/lib/iov_iter.c
>> @@ -1190,8 +1190,12 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
>>   		if (!n)
>>   			return -ENOMEM;
>>   		p = *pages;
>> -		for (int k = 0; k < n; k++)
>> -			get_page(p[k] = page + k);
>> +		for (int k = 0; k < n; k++) {
>> +			struct folio *folio = page_folio(page);
>> +			p[k] = page + k;
>> +			if (!folio_test_slab(folio))
>> +				folio_get(folio);
>> +		}
>>   		maxsize = min_t(size_t, maxsize, n * PAGE_SIZE - *start);
>>   		i->count -= maxsize;
>>   		i->iov_offset += maxsize;
>> 
> 
> Good news and bad news ...
> Good news: TLS works again!
> Bad news: no errors.

Wait, did you add a WARN_ON_ONCE() to the put_page() as I suggested? If yes
and there was no error, it would have to be leaking the page. Or the path
uses folio_put() and we'd need to put the warning there.

> Question to the wise: this is not the only place in iov_iter.c where we 
> do a 'get_page()'. Do we leave them and wait for others to report 
> regressions, knowing fully well that the current code _has_ issues?
> Or shouldn't we rather clean them up?
> 
> I guess the real fix would be to fiddle with the 'bio_add_page()' logic;
> we are always adding a 'page' reference to the bio, completely ignoring
> whether this page is a slab page or a normal one.
> 
> Discussion at LSF, maybe?
> 
> Cheers,
> 
> Hannes

Re: Kernel oops with 6.14 when enabling TLS

[Hannes Reinecke]

On 3/4/25 20:44, Vlastimil Babka wrote:
> On 3/4/25 20:39, Hannes Reinecke wrote:
>> On 3/4/25 19:05, Matthew Wilcox wrote:
>>> On Tue, Mar 04, 2025 at 04:53:09PM +0000, Matthew Wilcox wrote:
>>>> Right, that's what happened in the block layer.  We mark the bio with
>>>> BIO_PAGE_PINNED if the pincount needs to be dropped.  As a transitional
>>>> period, we had BIO_PAGE_REFFED which indicated that the page refcount
>>>> needed to be dropped.  Perhaps there's something similar that network
>>>> could be doing.
>>>
>>> Until that time ... how does this look as a quick hack to avoid
>>> reverting the slab change?
>>>
>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>> index d6fed25243c3..ca08a923ac6d 100644
>>> --- a/include/linux/mm.h
>>> +++ b/include/linux/mm.h
>>> @@ -1520,7 +1520,10 @@ static inline void folio_get(struct folio *folio)
>>>    
>>>    static inline void get_page(struct page *page)
>>>    {
>>> -	folio_get(page_folio(page));
>>> +	struct folio *folio = page_folio(page);
>>> +	if (WARN_ON_ONCE(folio_test_slab(folio)))
>>> +		return;
>>> +	folio_get(folio);
>>>    }
>>>    
>>>    static inline __must_check bool try_get_page(struct page *page)
>>> @@ -1614,6 +1617,8 @@ static inline void put_page(struct page *page)
>>>    {
>>>    	struct folio *folio = page_folio(page);
>>>    
>>> +	if (folio_test_slab(folio))
>>> +		return;
>>>    	folio_put(folio);
>>>    }
>>>    
>>> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
>>> index 65f550cb5081..8c7fdb7d8c8f 100644
>>> --- a/lib/iov_iter.c
>>> +++ b/lib/iov_iter.c
>>> @@ -1190,8 +1190,12 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i,
>>>    		if (!n)
>>>    			return -ENOMEM;
>>>    		p = *pages;
>>> -		for (int k = 0; k < n; k++)
>>> -			get_page(p[k] = page + k);
>>> +		for (int k = 0; k < n; k++) {
>>> +			struct folio *folio = page_folio(page);
>>> +			p[k] = page + k;
>>> +			if (!folio_test_slab(folio))
>>> +				folio_get(folio);
>>> +		}
>>>    		maxsize = min_t(size_t, maxsize, n * PAGE_SIZE - *start);
>>>    		i->count -= maxsize;
>>>    		i->iov_offset += maxsize;
>>>
>>
>> Good news and bad news ...
>> Good news: TLS works again!
>> Bad news: no errors.
> 
> Wait, did you add a WARN_ON_ONCE() to the put_page() as I suggested? If yes
> and there was no error, it would have to be leaking the page. Or the path
> uses folio_put() and we'd need to put the warning there.
> 
Oh, no, I didn't. Just added the WARN_ON to get_page().
Let me try ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: Kernel oops with 6.14 when enabling TLS

[Hannes Reinecke]

On 3/4/25 20:44, Vlastimil Babka wrote:
> On 3/4/25 20:39, Hannes Reinecke wrote:
[ .. ]
>>
>> Good news and bad news ...
>> Good news: TLS works again!
>> Bad news: no errors.
> 
> Wait, did you add a WARN_ON_ONCE() to the put_page() as I suggested? If yes
> and there was no error, it would have to be leaking the page. Or the path
> uses folio_put() and we'd need to put the warning there.
> 
That triggers:
[   42.364339] page dumped because: VM_WARN_ON_FOLIO(folio_test_slab(folio))
[   42.364379] ------------[ cut here ]------------
[   42.375500] WARNING: CPU: 0 PID: 236 at ./include/linux/mm.h:1564 
sk_msg_free_elem+0x157/0x180
[   42.375642] Modules linked in: tls(E) nvme_tcp(E) af_packet(E) 
iscsi_ibft(E) iscsi_boot_sysfs(E) xfs(E) nls_iso8859_1(E) nls_cp437(E) 
vfat(E) fat(E) iTCO_wdt(E) intel_pmc_bxt(E) intel_rapl_msr(E) 
iTCO_vendor_support(E) intel_rapl_common(E) i2c_i801(E) bnxt_en(E) 
i2c_mux(E) lpc_ich(E) mfd_core(E) i2c_smbus(E) virtio_balloon(E) 
joydev(E) button(E) nvme_fabrics(E) nvme_keyring(E) nvme_core(E) fuse(E) 
nvme_auth(E) efi_pstore(E) configfs(E) dmi_sysfs(E) ip_tables(E) 
x_tables(E) hid_generic(E) usbhid(E) ahci(E) libahci(E) libata(E) 
virtio_scsi(E) sd_mod(E) scsi_dh_emc(E) scsi_dh_rdac(E) scsi_dh_alua(E) 
qxl(E) sg(E) ghash_clmulni_intel(E) xhci_pci(E) drm_client_lib(E) 
drm_exec(E) drm_ttm_helper(E) sha512_ssse3(E) xhci_hcd(E) ttm(E) 
sha256_ssse3(E) drm_kms_helper(E) scsi_mod(E) sha1_ssse3(E) usbcore(E) 
scsi_common(E) drm(E) serio_raw(E) btrfs(E) blake2b_generic(E) xor(E) 
raid6_pq(E) efivarfs(E) qemu_fw_cfg(E) virtio_rng(E) aesni_intel(E) 
crypto_simd(E) cryptd(E)
[   42.393292] CPU: 0 UID: 0 PID: 236 Comm: kworker/0:1H Kdump: loaded 
Tainted: G            E      6.14.0-rc4-default+ #316 
cadaa81909a6170d00e1f47f3fc0db03c6a03650
[   42.393303] Tainted: [E]=UNSIGNED_MODULE
[   42.393305] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
0.0.0 02/06/2015
[   42.393310] Workqueue: nvme_tcp_wq nvme_tcp_io_work [nvme_tcp]
[   42.393323] RIP: 0010:sk_msg_free_elem+0x157/0x180
[   42.393331] Code: ff 48 c7 c6 d0 42 4f 82 48 89 ef e8 b3 63 8a ff 0f 
0b 48 8d 6a ff e9 6c ff ff ff 48 c7 c6 a0 42 4f 82 48 89 ef e8 99 63 8a 
ff <0f> 0b e9 c7 fe ff ff 2b 87 78 01 00 00 8b 97 c0 00 00 00 29 d0 ba
[   42.393336] RSP: 0018:ffffc9000040b798 EFLAGS: 00010282
[   42.393341] RAX: 000000000000003d RBX: ffff888110ab0858 RCX: 
0000000000000027
[   42.393344] RDX: 0000000000000000 RSI: 0000000000000002 RDI: 
ffff88817f423748
[   42.393347] RBP: ffffea0004295e00 R08: 0000000000000000 R09: 
0000000000000001
[   42.393350] R10: ffffc9000040b780 R11: ffffc9000040b4e0 R12: 
0000000000000400
[   42.393353] R13: ffff888110ab0818 R14: 0000000000000002 R15: 
ffff88810fa669d8
[   42.393361] FS:  0000000000000000(0000) GS:ffff88817f400000(0000) 
knlGS:0000000000000000
[   42.393365] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   42.393369] CR2: 00007f56a6ea6da4 CR3: 000000011bfc0000 CR4: 
0000000000350ef0
[   42.416071] Call Trace:
[   42.416078]  <TASK>
[   42.416084]  ? __warn+0x85/0x130
[   42.416095]  ? sk_msg_free_elem+0x157/0x180
[   42.418893]  ? report_bug+0xf8/0x1e0
[   42.418904]  ? handle_bug+0x50/0xa0
[   42.418910]  ? exc_invalid_op+0x13/0x60
[   42.418916]  ? asm_exc_invalid_op+0x16/0x20
[   42.418935]  ? sk_msg_free_elem+0x157/0x180
[   42.423206]  ? sk_msg_free_elem+0x157/0x180
[   42.423215]  __sk_msg_free+0x4f/0x100
[   42.423224]  tls_tx_records+0x118/0x190 [tls 
80cce2d02933ba636eb5845a829121ac309b44ed]
[   42.426506]  bpf_exec_tx_verdict+0x249/0x5e0 [tls 
80cce2d02933ba636eb5845a829121ac309b44ed]
[   42.426519]  ? srso_return_thunk+0x5/0x5f
[   42.426526]  ? __pfx_stack_trace_consume_entry+0x10/0x10
[   42.426572]  tls_sw_sendmsg+0x72f/0x9f0 [tls 
80cce2d02933ba636eb5845a829121ac309b44ed]
[   42.432016]  __sock_sendmsg+0x98/0xc0
[   42.432025]  sock_sendmsg+0x5c/0xa0
[   42.432030]  ? srso_return_thunk+0x5/0x5f
[   42.432034]  ? __sock_sendmsg+0x98/0xc0
[   42.432040]  ? srso_return_thunk+0x5/0x5f
[   42.436134]  ? sock_sendmsg+0x5c/0xa0
[   42.436146]  nvme_tcp_try_send_data+0x13f/0x410 [nvme_tcp 
9f4f1c84141d3edfcd3e478eb7c2fb638b4a92b3]
[   42.436159]  ? srso_return_thunk+0x5/0x5f
[   42.439452]  ? sched_balance_newidle+0x2f6/0x400
[   42.439468]  nvme_tcp_try_send+0x299/0x330 [nvme_tcp 
9f4f1c84141d3edfcd3e478eb7c2fb638b4a92b3]
[   42.439479]  nvme_tcp_io_work+0x37/0xb0 [nvme_tcp 
9f4f1c84141d3edfcd3e478eb7c2fb638b4a92b3]
[   42.443603]  process_scheduled_works+0x97/0x400
[   42.443614]  ? __pfx_worker_thread+0x10/0x10
[   42.443619]  worker_thread+0x105/0x240
[   42.443625]  ? __pfx_worker_thread+0x10/0x10
[   42.443630]  kthread+0xec/0x200
[   42.443639]  ? __pfx_kthread+0x10/0x10
[   42.443646]  ret_from_fork+0x30/0x50
[   42.443652]  ? __pfx_kthread+0x10/0x10
[   42.443658]  ret_from_fork_asm+0x1a/0x30
[   42.451127]  </TASK>
[   42.451131] ---[ end trace 0000000000000000 ]---

Not surprisingly, though, as the original code did a get_page(), so
there had to be a corresponding put_page() somewhere.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.com                               +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: Kernel oops with 6.14 when enabling TLS

[Vlastimil Babka]

On 3/5/25 09:20, Hannes Reinecke wrote:
> On 3/4/25 20:44, Vlastimil Babka wrote:
>> On 3/4/25 20:39, Hannes Reinecke wrote:
> [ .. ]
>>>
>>> Good news and bad news ...
>>> Good news: TLS works again!
>>> Bad news: no errors.
>> 
>> Wait, did you add a WARN_ON_ONCE() to the put_page() as I suggested? If yes
>> and there was no error, it would have to be leaking the page. Or the path
>> uses folio_put() and we'd need to put the warning there.
>> 
> That triggers:
...
> Not surprisingly, though, as the original code did a get_page(), so
> there had to be a corresponding put_page() somewhere.

Is is this one? If there's no more warning afterwards, that should be it.

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 61f3f3d4e528..b37d99cec069 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -182,9 +182,14 @@ static int sk_msg_free_elem(struct sock *sk, struct sk_msg *msg, u32 i,
 
        /* When the skb owns the memory we free it from consume_skb path. */
        if (!msg->skb) {
+               struct folio *folio;
+
                if (charge)
                        sk_mem_uncharge(sk, len);
-               put_page(sg_page(sge));
+
+               folio = page_folio(sg_page(sge));
+               if (!folio_test_slab(folio))
+                       folio_put(folio);
        }
        memset(sge, 0, sizeof(*sge));
        return len;

Re: Kernel oops with 6.14 when enabling TLS

[Hannes Reinecke]

On 3/5/25 09:58, Vlastimil Babka wrote:
> On 3/5/25 09:20, Hannes Reinecke wrote:
>> On 3/4/25 20:44, Vlastimil Babka wrote:
>>> On 3/4/25 20:39, Hannes Reinecke wrote:
>> [ .. ]
>>>>
>>>> Good news and bad news ...
>>>> Good news: TLS works again!
>>>> Bad news: no errors.
>>>
>>> Wait, did you add a WARN_ON_ONCE() to the put_page() as I suggested? If yes
>>> and there was no error, it would have to be leaking the page. Or the path
>>> uses folio_put() and we'd need to put the warning there.
>>>
>> That triggers:
> ...
>> Not surprisingly, though, as the original code did a get_page(), so
>> there had to be a corresponding put_page() somewhere.
> 
> Is is this one? If there's no more warning afterwards, that should be it.
> 
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 61f3f3d4e528..b37d99cec069 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -182,9 +182,14 @@ static int sk_msg_free_elem(struct sock *sk, struct sk_msg *msg, u32 i,
>   
>          /* When the skb owns the memory we free it from consume_skb path. */
>          if (!msg->skb) {
> +               struct folio *folio;
> +
>                  if (charge)
>                          sk_mem_uncharge(sk, len);
> -               put_page(sg_page(sge));
> +
> +               folio = page_folio(sg_page(sge));
> +               if (!folio_test_slab(folio))
> +                       folio_put(folio);
>          }
>          memset(sge, 0, sizeof(*sge));
>          return len;
> 
> 
Oh, sure. But what annoys me: why do we have to care?

When doing I/O _all_ data is stuffed into bvecs via
bio_add_page(), and after that information about the
origin is lost; any iteration on the bio will be a bvec
iteration.
Previously we could just do a bvec iteration, get a reference
for each page, and start processing.
Now suddenly the caller has to check if it's a slab page and don't
get a reference for that. Not only that, he also has to remember
to _not_ drop the reference when he's done.
And, of course, tracing get_page() and the corresponding put_page()
calls through all the layers.
Really?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Networking people smell funny and make poor life choices

[Matthew Wilcox]

On Wed, Mar 05, 2025 at 12:43:02PM +0100, Hannes Reinecke wrote:
> Oh, sure. But what annoys me: why do we have to care?
> 
> When doing I/O _all_ data is stuffed into bvecs via
> bio_add_page(), and after that information about the
> origin is lost; any iteration on the bio will be a bvec
> iteration.
> Previously we could just do a bvec iteration, get a reference
> for each page, and start processing.
> Now suddenly the caller has to check if it's a slab page and don't
> get a reference for that. Not only that, he also has to remember
> to _not_ drop the reference when he's done.
> And, of course, tracing get_page() and the corresponding put_page()
> calls through all the layers.

Networking needs to follow block's lead and STOP GETTING REFCOUNTS ON
PAGES.  That will speed up networking (eliminates two atomic operations per
page).  And of course, it will eliminate this hack in the MM.  I think
we do need to put this hack into the MM for now, but it needs to go away
again as quickly as possible.

What worries me is that nobody in networking has replied to this thread
yet.  Do they not care?  Let's see if a subject line change will help
with that.

Re: Networking people smell funny and make poor life choices

[Cong Wang]

On Wed, Mar 05, 2025 at 06:11:24PM +0000, Matthew Wilcox wrote:
> On Wed, Mar 05, 2025 at 12:43:02PM +0100, Hannes Reinecke wrote:
> > Oh, sure. But what annoys me: why do we have to care?
> > 
> > When doing I/O _all_ data is stuffed into bvecs via
> > bio_add_page(), and after that information about the
> > origin is lost; any iteration on the bio will be a bvec
> > iteration.
> > Previously we could just do a bvec iteration, get a reference
> > for each page, and start processing.
> > Now suddenly the caller has to check if it's a slab page and don't
> > get a reference for that. Not only that, he also has to remember
> > to _not_ drop the reference when he's done.
> > And, of course, tracing get_page() and the corresponding put_page()
> > calls through all the layers.
> 
> Networking needs to follow block's lead and STOP GETTING REFCOUNTS ON
> PAGES.  That will speed up networking (eliminates two atomic operations per
> page).  And of course, it will eliminate this hack in the MM.  I think
> we do need to put this hack into the MM for now, but it needs to go away
> again as quickly as possible.
> 
> What worries me is that nobody in networking has replied to this thread
> yet.  Do they not care?  Let's see if a subject line change will help
> with that.

Since it triggered a kernel crash, I am pretty sure people care. How
about sending out a patch to get more attentions?

I am not sure what patterns here you are suggesting to change w.r.t page
refcount, but at least using AI copilot or whatever automation tool should
be very handy.

Thanks.

Re: Networking people smell funny and make poor life choices

[Christoph Hellwig]

On Wed, Mar 05, 2025 at 06:11:24PM +0000, Matthew Wilcox wrote:
> Networking needs to follow block's lead and STOP GETTING REFCOUNTS ON
> PAGES.

The block layer never took references on pages.  The direct I/O helpers
that just happened to set in block/ did hold references and abused some
field in the bio for it (and still do for the pinning), but the reference
was (and the pin now is) owned by the submitter.

The block layer model has always been that the submitter needs to ensure
memory stays allocated until the I/O has completed.  Which IMHO is the
only sane model for dealing with memory lifetimes vs I/O, and something
networking absolutely should follow.

RE: Networking people smell funny and make poor life choices

[James R. Bergsten]

OK another "unnecessary" old-timer storage/network story/disruption to your otherwise relevant discussions (thank the subject line). If you're too busy, just don't read it. 😊

Around 1985, Gene Amdahl founded a company called Andor. Its original purpose (as was with everything Gene did) was to build the smallest plug-compatible mainframe.  When it was designed, someone noted it had no physical room for the humongous "Bus and Tag" cables needed for peripherals, so Gene raised a bit more money and started a storage project too.

When the Loma Prieta earthquake happened in 1989, PG&E, the local utility, lost the datacenter containing all of the information needed to repair their utilities, so the service people had to do this from memory.  The Public Utilities Commission didn't find this terribly funny, so they said PG&E had to create a second datacenter out of the area immediately and have backups there within about 24 hours, shorter as time went on.  So, they shut down the primary site every night, dumped to tape, then drove it up to Sacramento where these were restored.  They named this CTAM for "Chevy Truck Access Method."

Somehow Gene and friends heard about this and, as they already had a processor, device simulation and devices, if they added some sort of networking interface, they could have a local unit and a remote unit doing this backup, eliminating the truck.  BTW the "front end" storage group all came from Memorex.  The "back end" group mostly from Amdahl.

This actually (somewhat) worked, and a couple of units were installed in beta sites.  Sadly, Gene ran out of money (or at least didn't accept the terms offered) and buggered off to start yet another mainframe company which never shipped anything.

I was the last Engineering VP at Andor, so when it folded, I grabbed a few of the people and started a similar company but for the open systems market instead.  We named it "Ark" at my wife's suggestion as was like Noah's Ark - "disaster recovery" and "two of everything."  We mostly bootstrapped, did ship product, and were acquired by LSI Logic who were getting beaten around the head as EMC had a remote solution, but LSI didn't. I got about a dozen US Patents Issued and enough money to finally buy a house in Silicon Valley.

Our (SCSI-based) device had front end ports for the host(s), back-end ones for the devices, and side ones for the networking.  Lots of features, some you folks are only doing recently.  Looked like devices to hosts, hosts to devices.

Anyway, the point of all of this is that when we sold it to customers, the storage people looked at the network ports with confusion and dismay (some hadn't even ordered the network lines and caused months of delay), while the network people looked at the device ports as if they were full of Tasmanian devils.

Turned out, both network and storage expertise were very rare commodities.  This was largely why most iSCSI startups failed, they either did a storage product or a networking product. We pilled this off because I am stupid but stubborn and wrote the RTOS myself (Linux was in its infancy and the other RTOS's sucked).  Seemed a good idea at the time.  Have white papers online if anybody is interested.

So, networking people may smell funny, but to them storage people come from another galaxy.  Working in this industry at all could be considered a poor life choice but that's for another time.

Sorry. You can go back to work now.
Jim B

-----Original Message-----
From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> On Behalf Of Christoph Hellwig
Sent: Wednesday, March 12, 2025 8:09 AM
To: Matthew Wilcox <willy@infradead.org>
Cc: Hannes Reinecke <hare@suse.de>; Vlastimil Babka <vbabka@suse.cz>; Hannes Reinecke <hare@suse.com>; Boris Pismenny <borisp@nvidia.com>; John Fastabend <john.fastabend@gmail.com>; Jakub Kicinski <kuba@kernel.org>; Sagi Grimberg <sagi@grimberg.me>; linux-nvme@lists.infradead.org; linux-block@vger.kernel.org; linux-mm@kvack.org; Harry Yoo <harry.yoo@oracle.com>; netdev@vger.kernel.org
Subject: Re: Networking people smell funny and make poor life choices

On Wed, Mar 05, 2025 at 06:11:24PM +0000, Matthew Wilcox wrote:
> Networking needs to follow block's lead and STOP GETTING REFCOUNTS ON 
> PAGES.

The block layer never took references on pages.  The direct I/O helpers that just happened to set in block/ did hold references and abused some field in the bio for it (and still do for the pinning), but the reference was (and the pin now is) owned by the submitter.

The block layer model has always been that the submitter needs to ensure memory stays allocated until the I/O has completed.  Which IMHO is the only sane model for dealing with memory lifetimes vs I/O, and something networking absolutely should follow.

Re: Networking people smell funny and make poor life choices

[David Laight]

On Wed, 5 Mar 2025 18:11:24 +0000
Matthew Wilcox <willy@infradead.org> wrote:


> What worries me is that nobody in networking has replied to this thread
> yet.  Do they not care?  Let's see if a subject line change will help
> with that.
> 

I like being smelly :-(

Re: Kernel oops with 6.14 when enabling TLS

[Vlastimil Babka]

On 3/5/25 12:43, Hannes Reinecke wrote:
> On 3/5/25 09:58, Vlastimil Babka wrote:
>> On 3/5/25 09:20, Hannes Reinecke wrote:
>>> On 3/4/25 20:44, Vlastimil Babka wrote:
>>>> On 3/4/25 20:39, Hannes Reinecke wrote:
>>> [ .. ]
>>>>>
>>>>> Good news and bad news ...
>>>>> Good news: TLS works again!
>>>>> Bad news: no errors.
>>>>
>>>> Wait, did you add a WARN_ON_ONCE() to the put_page() as I suggested? If yes
>>>> and there was no error, it would have to be leaking the page. Or the path
>>>> uses folio_put() and we'd need to put the warning there.
>>>>
>>> That triggers:
>> ...
>>> Not surprisingly, though, as the original code did a get_page(), so
>>> there had to be a corresponding put_page() somewhere.
>> 
>> Is is this one? If there's no more warning afterwards, that should be it.
>> 
>> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
>> index 61f3f3d4e528..b37d99cec069 100644
>> --- a/net/core/skmsg.c
>> +++ b/net/core/skmsg.c
>> @@ -182,9 +182,14 @@ static int sk_msg_free_elem(struct sock *sk, struct sk_msg *msg, u32 i,
>>   
>>          /* When the skb owns the memory we free it from consume_skb path. */
>>          if (!msg->skb) {
>> +               struct folio *folio;
>> +
>>                  if (charge)
>>                          sk_mem_uncharge(sk, len);
>> -               put_page(sg_page(sge));
>> +
>> +               folio = page_folio(sg_page(sge));
>> +               if (!folio_test_slab(folio))
>> +                       folio_put(folio);
>>          }
>>          memset(sge, 0, sizeof(*sge));
>>          return len;
>> 
>> 
> Oh, sure. But what annoys me: why do we have to care?
> 
> When doing I/O _all_ data is stuffed into bvecs via
> bio_add_page(), and after that information about the
> origin is lost; any iteration on the bio will be a bvec
> iteration.
> Previously we could just do a bvec iteration, get a reference
> for each page, and start processing.

AFAIU there's BIO_PAGE_PINNED that controls whether the pages are pinned, as
there are usecases where it makes sense to do that (userspace pages?). And
__bio_release_pages() can be removing the last pin and freeing the pages.

But this is a case where the buffer is a kmalloc() allocation, so somebody
has to do the corresponding kfree() when the messages are processed. A pin
on the slab folio where the kmalloc() resides helps nothing and as willy
says it's just unnecessary overhead of atomic allocations.

> Now suddenly the caller has to check if it's a slab page and don't
> get a reference for that. Not only that, he also has to remember
> to _not_ drop the reference when he's done.

The caller did kmalloc() and will have to do kfree(). I guess it's about
telling the intermediate layers via something similar like BIO_PAGE_PINNED
whether the pages should be pinned or not.

> And, of course, tracing get_page() and the corresponding put_page()
> calls through all the layers.
> Really?
> 
> Cheers,
> 
> Hannes