| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAL+tcoBsfxyfGUyKfuiQsqwc8useNefZWxSVJOyivEci9jM6Zw@mail.gmail.com>
Date: Tue, 2 Sep 2025 22:04:54 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Xin Zhao <jackzxcui1989@....com>
Cc: willemdebruijn.kernel@...il.com, edumazet@...gle.com, ferenc@...es.dev,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v10 1/2] net: af_packet: remove
last_kactive_blk_num field
On Sun, Aug 31, 2025 at 6:10 PM Xin Zhao <jackzxcui1989@....com> wrote:
>
> kactive_blk_num (K) is incremented on block close. last_kactive_blk_num (L)
> is set to match K on block open and each timer. So the only time that they
> differ is if a block is closed in tpacket_rcv and no new block could be
> opened.
> So the origin check L==K in timer callback only skip the case 'no new block
> to open'. If we remove L==K check, it will make prb_curr_blk_in_use check
> earlier, which will not cause any side effect.
I believe the above commit message needs to be revised:
1) the above sentence (starting from 'if we remove L....') means
nothing because your modification doesn't change the behaviour when
the queue is not frozen.
2) lack of proofs/reasons on why exposing the prb_open_block() logic doesn't
cause side effects. It's the key proof that shows to future readers to
make sure this patch will not bring trouble.
>
> Signed-off-by: Xin Zhao <jackzxcui1989@....com>
It was suggested by Willem, so please add:
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@...il.com>
So far, it looks good to me as well:
Reviewed-by: Jason Xing <kerneljasonxing@...il.com>
And I will finish reviewing the other patch by tomorrow :)
Thanks,
Jason
> ---
> net/packet/af_packet.c | 60 ++++++++++++++++++++----------------------
> net/packet/internal.h | 6 -----
> 2 files changed, 28 insertions(+), 38 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index a7017d7f0..d4eb4a4fe 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -669,7 +669,6 @@ static void init_prb_bdqc(struct packet_sock *po,
> p1->knum_blocks = req_u->req3.tp_block_nr;
> p1->hdrlen = po->tp_hdrlen;
> p1->version = po->tp_version;
> - p1->last_kactive_blk_num = 0;
> po->stats.stats3.tp_freeze_q_cnt = 0;
> if (req_u->req3.tp_retire_blk_tov)
> p1->retire_blk_tov = req_u->req3.tp_retire_blk_tov;
> @@ -693,7 +692,6 @@ static void _prb_refresh_rx_retire_blk_timer(struct tpacket_kbdq_core *pkc)
> {
> mod_timer(&pkc->retire_blk_timer,
> jiffies + pkc->tov_in_jiffies);
> - pkc->last_kactive_blk_num = pkc->kactive_blk_num;
> }
>
> /*
> @@ -750,38 +748,36 @@ static void prb_retire_rx_blk_timer_expired(struct timer_list *t)
> write_unlock(&pkc->blk_fill_in_prog_lock);
> }
>
> - if (pkc->last_kactive_blk_num == pkc->kactive_blk_num) {
> - if (!frozen) {
> - if (!BLOCK_NUM_PKTS(pbd)) {
> - /* An empty block. Just refresh the timer. */
> - goto refresh_timer;
> - }
> - prb_retire_current_block(pkc, po, TP_STATUS_BLK_TMO);
> - if (!prb_dispatch_next_block(pkc, po))
> - goto refresh_timer;
> - else
> - goto out;
> + if (!frozen) {
> + if (!BLOCK_NUM_PKTS(pbd)) {
> + /* An empty block. Just refresh the timer. */
> + goto refresh_timer;
> + }
> + prb_retire_current_block(pkc, po, TP_STATUS_BLK_TMO);
> + if (!prb_dispatch_next_block(pkc, po))
> + goto refresh_timer;
> + else
> + goto out;
> + } else {
> + /* Case 1. Queue was frozen because user-space was
> + * lagging behind.
> + */
> + if (prb_curr_blk_in_use(pbd)) {
> + /*
> + * Ok, user-space is still behind.
> + * So just refresh the timer.
> + */
> + goto refresh_timer;
> } else {
> - /* Case 1. Queue was frozen because user-space was
> - * lagging behind.
> + /* Case 2. queue was frozen,user-space caught up,
> + * now the link went idle && the timer fired.
> + * We don't have a block to close.So we open this
> + * block and restart the timer.
> + * opening a block thaws the queue,restarts timer
> + * Thawing/timer-refresh is a side effect.
> */
> - if (prb_curr_blk_in_use(pbd)) {
> - /*
> - * Ok, user-space is still behind.
> - * So just refresh the timer.
> - */
> - goto refresh_timer;
> - } else {
> - /* Case 2. queue was frozen,user-space caught up,
> - * now the link went idle && the timer fired.
> - * We don't have a block to close.So we open this
> - * block and restart the timer.
> - * opening a block thaws the queue,restarts timer
> - * Thawing/timer-refresh is a side effect.
> - */
> - prb_open_block(pkc, pbd);
> - goto out;
> - }
> + prb_open_block(pkc, pbd);
> + goto out;
> }
> }
>
> diff --git a/net/packet/internal.h b/net/packet/internal.h
> index 1e743d031..d367b9f93 100644
> --- a/net/packet/internal.h
> +++ b/net/packet/internal.h
> @@ -24,12 +24,6 @@ struct tpacket_kbdq_core {
> unsigned short kactive_blk_num;
> unsigned short blk_sizeof_priv;
>
> - /* last_kactive_blk_num:
> - * trick to see if user-space has caught up
> - * in order to avoid refreshing timer when every single pkt arrives.
> - */
> - unsigned short last_kactive_blk_num;
> -
> char *pkblk_start;
> char *pkblk_end;
> int kblk_size;
> --
> 2.34.1
>
>
Powered by blists - more mailing lists