Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201007101726.3149375-1-a.nogikh@gmail.com> <bec6415925c213a2e3eb86e80d6982b82180f019.camel@sipsolutions.net>
In-Reply-To: <bec6415925c213a2e3eb86e80d6982b82180f019.camel@sipsolutions.net>
From:   Aleksandr Nogikh <a.nogikh@gmail.com>
Date:   Wed, 7 Oct 2020 17:40:52 +0300
Message-ID: <CADpXja8oh2nHpNFjQ-+iDUebPx7R_HcOuoakzmKvHp=kuj8rpA@mail.gmail.com>
Subject: Re: [PATCH 0/2] net, mac80211: enable KCOV remote coverage collection
 for 802.11 frame handling
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     davem@davemloft.net, kuba@kernel.org,
        Eric Dumazet <edumazet@google.com>,
        Andrey Konovalov <andreyknvl@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Marco Elver <elver@google.com>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, linux-wireless@vger.kernel.org,
        nogikh@google.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 7 Oct 2020 at 14:48, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Wed, 2020-10-07 at 10:17 +0000, Aleksandr Nogikh wrote:
[...]
> > Aleksandr Nogikh (2):
> >   net: store KCOV remote handle in sk_buff
>
> Can you explain that a bit better? What is a "remote handle"? What does
> it do in the SKB?
>
> I guess I'd have to know more about "kcov_common_handle()" to understand
> this bit.

Normally, KCOV collects coverage information for the code that is
executed inside the system call context. It is easy to identify where
that coverage should go and whether it should be collected at all by
looking at the current process. If KCOV was enabled on that process,
coverage will be stored in a buffer specific to that process.
Howerever, it is not always enough as some handling can happen
elsewhere (e.g. in separate kernel threads).

That is why remote KOV coverage collection was introduced. When it is
impossible to infer KCOV-related info just by looking at the currently
running process, we need to manually pass some information to the code
that is of interest to us.  The information takes the form of 64 bit
integers (remote handles). Zero is the special value that corresponds
to an empty handle. More details on KCOV and remote coverage
collection can be found here: Documentation/dev-tools/kcov.rst.

In this patch, we obtain the remote handle from KCOV (in this case by
executing kcov_common_handle()) and attach it to newly allocated
SKBs. If we're in a system call context, the SKB will be tied to the
process that issued the syscall (if that process is interested in
remote coverage collection). So when
kcov_remote_start_common(skb_get_kcov_handle(skb)) is executed, it is
possible to determine whether coverage is required and where it should
be stored.

I have just realized that the default kcov_handle initialization as it
was implemented in this patch is not really robust. If an skb is
allocated during a hard IRQ, kcov_common_handle() will return a remote
handle for the interrupted thread instead of returning 0, and that is
not desirable since it will occasionally lead to wrong kcov_handles. I
will fix it in the next version of the patch.

> >   mac80211: add KCOV remote annotations to incoming frame processing
>
> This seems fine, but a bit too limited? You tagged
> only ieee80211_tasklet_handler() which calls ieee80211_rx()
> or ieee80211_tx_status(), but
>
> 1) I'm not even sure ieee80211_tx_status() counts (it's processing
> locally generated frames after they round-tripped into the driver
> (although in mesh it could be remote originated but retransmitted
> frames, so I guess it makes some sense?); and
>
> 2) there are many other ways that ieee80211_rx() could get called.
>
> It seems to me it'd make more sense to (also) annotate ieee80211_rx()
> itself?

Yes, it definitely makes more sense to annotate ieee80211_rx()
directly. Collecting coverage for ieee80211_tx_status() does not seem
to be needed now and can be added later if there's a use case for it.

Thank you for the suggestion. I will implement it in the second
version of the patch.

--
Best regards,
Aleksandr