linux-kernel - Re: [PATCH net-next v5] net/sock: Introduce trace_sk_data

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] [day] [month] [year] [list]

Date:   Wed, 9 Nov 2022 18:28:22 -0800
From:   Peilin Ye <yepeilin.cs@...il.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Peilin Ye <peilin.ye@...edance.com>,
        Cong Wang <cong.wang@...edance.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v5] net/sock: Introduce trace_sk_data_ready()

On Fri, Oct 14, 2022 at 09:35:22AM +0300, Leon Romanovsky wrote:
> Please don't reply-to new patches and always send them as new threads
> with links to previous versions in changelog.

Sure, I can do that.  However, for example: 

  - I got a build error.
  - I found on LKML some v1 patch developed by someone else that
    introduced a similar error, by searching the error message.
  - I wanted to know how v2 fixed that error in v1, but since v2 didn't
    in-reply-to v1, it took me some extra seconds to find v2.

Therefore, I think sometimes it's useful to keep all versions in one
thread, especially when the set only contains one patch?

> > diff --git a/drivers/infiniband/hw/erdma/erdma_cm.c b/drivers/infiniband/hw/erdma/erdma_cm.c
> > index f13f16479eca..084da6698080 100644
> > --- a/drivers/infiniband/hw/erdma/erdma_cm.c
> > +++ b/drivers/infiniband/hw/erdma/erdma_cm.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/types.h>
> >  #include <linux/workqueue.h>
> >  #include <net/addrconf.h>
> > +#include <trace/events/sock.h>
> >  
> >  #include <rdma/ib_user_verbs.h>
> >  #include <rdma/ib_verbs.h>
> > @@ -933,6 +934,8 @@ static void erdma_cm_llp_data_ready(struct sock *sk)
> >  {
> >  	struct erdma_cep *cep;
> >  
> > +	trace_sk_data_ready(sk);
> > +
> >  	read_lock(&sk->sk_callback_lock);
> 
> I see this pattern in all places and don't know if it is correct or not,
> but you are calling to trace_sk_data_ready() at the beginning of
> function and do it without taking sk_callback_lock.

Thanks for bringing this up, but I'm not sure it's an issue.  We already
do similar thing, for example, in net/core/neighbour.c:

static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
			  u8 new, u32 flags, u32 nlmsg_pid,
			  struct netlink_ext_ack *extack)
{
	bool gc_update = false, managed_update = false;
	int update_isrouter = 0;
	struct net_device *dev;
	int err, notify = 0;
	u8 old;

	trace_neigh_update(neigh, lladdr, new, flags, nlmsg_pid);

	write_lock_bh(&neigh->lock);

Thanks,
Peilin Ye