netdev - Re: [PATCH v1 net-next 3/6] net: add new control message for incoming HW-timestamped packets

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: <CA+FuTScHMLmmM+=50C+yutXCk56YzXE=ymg_3whFqG6w90aU5A@mail.gmail.com>
Date:   Thu, 27 Apr 2017 07:38:04 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Miroslav Lichvar <mlichvar@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Soheil Hassas Yeganeh <soheil@...gle.com>,
        "Keller, Jacob E" <jacob.e.keller@...el.com>,
        Denny Page <dennypage@...com>, Jiri Benc <jbenc@...hat.com>
Subject: Re: [PATCH v1 net-next 3/6] net: add new control message for incoming
 HW-timestamped packets

On Thu, Apr 27, 2017 at 6:15 AM, Miroslav Lichvar <mlichvar@...hat.com> wrote:
> Thanks for the comments.
>
> On Wed, Apr 26, 2017 at 07:34:49PM -0400, Willem de Bruijn wrote:
>> > +struct net_device *dev_get_by_napi_id(unsigned int napi_id)
>> > +{
>> > +       struct net_device *dev = NULL;
>> > +       struct napi_struct *napi;
>> > +
>> > +       rcu_read_lock();
>> > +
>> > +       napi = napi_by_id(napi_id);
>> > +       if (napi)
>> > +               dev = napi->dev;
>> > +
>> > +       rcu_read_unlock();
>> > +
>> > +       return dev;
>> > +}
>> > +EXPORT_SYMBOL(dev_get_by_napi_id);
>>
>> Returning dev without holding a reference is not safe. You'll probably
>> have to call this with rcu_read_lock held instead.
>
> How about changing the function to simply return the index instead of
> the device (e.g. dev_get_ifindex_by_napi_id())? Would that be too
> specific?

I suspect so. If you can achieve the same by calling this function
within an rcu read_side critical section, I would do that.

>> >  /*
>> >   * called from sock_recv_timestamp() if sock_flag(sk, SOCK_RCVTSTAMP)
>> >   */
>> > @@ -699,8 +719,12 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>> >                 empty = 0;
>> >         if (shhwtstamps &&
>> >             (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>>
>> This information is also informative with software timestamps.
>
> But is it useful and worth the cost? If I have two interfaces and only
> one has HW timestamping, or just one interface which can timestamp
> incoming packets at a limited rate, I would prefer to not waste CPU
> cycles preparing and processing useless data.
>
>> And getting the real iif is definitely useful outside timestamps.
>
> Do you have an example? We have asked that in the original thread,
> but no one suggested anything. For AF_PACKET there is PACKET_ORIGDEV.
> When I was searching the Internet on how to get the index with INET
> sockets, it looked like I was the only one who had this problem :).

Okay. Maybe I'm mistaken. If no one else responds with a use case,
then I agree that we can ignore these cases.

>> An
>> alternative approach is to add versioning to IP_PKTINFO with a new
>> setsockopt IP_PKTINFO_VERSION plus a new struct in_pktinfo_v2
>> that extends in_pktinfo. Just a thought.
>
> The struct would contain both the original and last interface index,
> and the length as well? And similarly with in6_pktinfo?
>
> If there is an agreement that the information would useful also for
> other things than timestamping, I can try that. If not, I think it
> would be better to keep it tied to HW timestamping.

Ack.