linux-kernel - Re: [PATCH 05/21] can: bcm: Don't initialized an unused hrtimer

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <203a1560-76c9-4c47-b1b8-b44ddf40cd16@hartkopp.net>
Date: Wed, 30 Oct 2024 11:49:49 +0100
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Nam Cao <namcao@...utronix.de>,
 Anna-Maria Behnsen <anna-maria@...utronix.de>,
 Frederic Weisbecker <frederic@...nel.org>,
 Thomas Gleixner <tglx@...utronix.de>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Miguel Ojeda <ojeda@...nel.org>, Kees Cook <kees@...nel.org>,
 linux-kernel@...r.kernel.org
Cc: Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH 05/21] can: bcm: Don't initialized an unused hrtimer



On 28.10.24 08:29, Nam Cao wrote:
> The hrtimer "thrtimer" is not used for TX. But this timer is initialized
> regardless.
> 
> Remove the hrtimer_init() for the unused hrtimer and change bcm_remove_op()
> to make sure hrtimer_cancel() is not called with the uninitialized hrtimer.

NAK.

There are several other occurrences of thrtimer that are not covered by 
RX/TX distinction, where the second timer is canceled.

This one-time init and cancel of an unused hrtimer costs nearly nothing 
and is not even in any hot path.

So this incomplete patch only adds complexity and potential error cases 
in some 20 y/o code for nothing.

Therefore I would like to skip it.

Thanks,
Oliver

> 
> Signed-off-by: Nam Cao <namcao@...utronix.de>
> ---
> Cc: Oliver Hartkopp <socketcan@...tkopp.net>
> Cc: Jakub Kicinski <kuba@...nel.org>
> ---
>   net/can/bcm.c | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 217049fa496e..792528f7bce2 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -780,10 +780,11 @@ static void bcm_free_op_rcu(struct rcu_head *rcu_head)
>   	kfree(op);
>   }
>   
> -static void bcm_remove_op(struct bcm_op *op)
> +static void bcm_remove_op(struct bcm_op *op, bool is_tx)
>   {
>   	hrtimer_cancel(&op->timer);
> -	hrtimer_cancel(&op->thrtimer);
> +	if (!is_tx)
> +		hrtimer_cancel(&op->thrtimer);
>   
>   	call_rcu(&op->rcu, bcm_free_op_rcu);
>   }
> @@ -844,7 +845,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
>   						  bcm_rx_handler, op);
>   
>   			list_del(&op->list);
> -			bcm_remove_op(op);
> +			bcm_remove_op(op, false);
>   			return 1; /* done */
>   		}
>   	}
> @@ -864,7 +865,7 @@ static int bcm_delete_tx_op(struct list_head *ops, struct bcm_msg_head *mh,
>   		if ((op->can_id == mh->can_id) && (op->ifindex == ifindex) &&
>   		    (op->flags & CAN_FD_FRAME) == (mh->flags & CAN_FD_FRAME)) {
>   			list_del(&op->list);
> -			bcm_remove_op(op);
> +			bcm_remove_op(op, true);
>   			return 1; /* done */
>   		}
>   	}
> @@ -1015,10 +1016,6 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>   			     HRTIMER_MODE_REL_SOFT);
>   		op->timer.function = bcm_tx_timeout_handler;
>   
> -		/* currently unused in tx_ops */
> -		hrtimer_init(&op->thrtimer, CLOCK_MONOTONIC,
> -			     HRTIMER_MODE_REL_SOFT);
> -
>   		/* add this bcm_op to the list of the tx_ops */
>   		list_add(&op->list, &bo->tx_ops);
>   
> @@ -1277,7 +1274,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>   		if (err) {
>   			/* this bcm rx op is broken -> remove it */
>   			list_del(&op->list);
> -			bcm_remove_op(op);
> +			bcm_remove_op(op, false);
>   			return err;
>   		}
>   	}
> @@ -1581,7 +1578,7 @@ static int bcm_release(struct socket *sock)
>   #endif /* CONFIG_PROC_FS */
>   
>   	list_for_each_entry_safe(op, next, &bo->tx_ops, list)
> -		bcm_remove_op(op);
> +		bcm_remove_op(op, true);
>   
>   	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
>   		/*
> @@ -1613,7 +1610,7 @@ static int bcm_release(struct socket *sock)
>   	synchronize_rcu();
>   
>   	list_for_each_entry_safe(op, next, &bo->rx_ops, list)
> -		bcm_remove_op(op);
> +		bcm_remove_op(op, false);
>   
>   	/* remove device reference */
>   	if (bo->bound) {