netdev - Re: Netlink NLM_F_DUMP

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]

Message-ID: <20220824114628.GA14776@breakpoint.cc>
Date:   Wed, 24 Aug 2022 13:46:28 +0200
From:   Florian Westphal <fw@...len.de>
To:     Ismael Luceno <iluceno@...e.de>
Cc:     David Ahern <dsahern@...il.com>, Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Florian Westphal <fw@...len.de>
Subject: Re: Netlink NLM_F_DUMP_INTR flag lost

Ismael Luceno <iluceno@...e.de> wrote:
> It seems to me now that most of the calls to nl_dump_check_consistent
> are redundant.
> 
> Either the rtnl lock is explicitly taken:
> - ethnl_tunnel_info_dumpit

That guarantess *this* invocation of ethnl_tunnel_info_dumpit doesn't
change in between, but it doesn't guarantee consistency of the dump.

rtn_lock();
cb->seq = 1
 dumping...
 skb full
unlock();

nl_dump_check_consistent();
return-to-userspace
 /* write happens, base seq increments */

next recv():
rtn_lock();
seq is now > 1
 resume dumping from pos h
unlock();
nl_dump_check_consistent(); -> prev_seq is 1 but seq > 1 -> set INTR

It doesn't really matter if dumpit() grabs RTNL or another lock or no
lock at all (rcu) unless there is a guarantee that everything will fit
in one recv() call. I am not aware of such a guarantee anywhere.

If you meant that there is another nl_dump_check_consistent() that
already covers this then I missed it.

> I assume the ones that rely on rcu_read_lock are safe too.
> 
> Also, the following ones set cb->seq just before calling it:
> - rtm_dump_nh_ctx
> - rtm_dump_res_bucket_ctx

I'm not sure what you mean, do you mean

3443 out_err:
3444         cb->seq = net->nexthop.seq;
3445         nl_dump_check_consistent(cb, nlmsg_hdr(skb));
3446         return err;
3447 }

I don't see why this is buggy.  rtm_dump_nexthop_bucket() is called
with RTNL held.  Things were different in case of RCU, because we'd miss
flagging INTR in case change happened while doing the first/initial dump
invocation.

i.e.:
- dump starts
   // parallel modification, seq increments
- set seq to *incremented* number
- prev_seq is 0,

End of next round sees seq == incremented && prev_seq == seq, no INTR
set.

For the RCU case, seq needs to be set before dump starts.
For RTNL, no parallel modifications can happen, so the above is fine.

Modification can only happen after unlock, so next dump will see
prev_seq != seq and sets the INTR flag.