netdev - Re: [PATCH net-next 7/7] selftests/net: add zerocopy support for PF

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 for Android: free password hash cracker in your pocket

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

Date:   Sun, 28 Jan 2018 19:39:51 +0100
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>, rds-devel@....oracle.com,
        santosh.shilimkar@...cle.com
Subject: Re: [PATCH net-next 7/7] selftests/net: add zerocopy support for
 PF_RDS test case

On Sun, Jan 28, 2018 at 5:18 PM, Sowmini Varadhan
<sowmini.varadhan@...cle.com> wrote:
>
> Re-ordering review comments for selftests a bit..
>
>> > +       cm->cmsg_level = SOL_RDS;
>> > +       cm->cmsg_type = RDS_CMSG_ZCOPY_COOKIE;
>> > +       ++cookie;
>> > +       memcpy(CMSG_DATA(cm), &cookie, sizeof(cookie));
>> cookie is not initialized
>
> it's a static uint32_t in the function. It will get initialized to 0.

Oh right. I missed that.

> But the whole test program is rather simplistic, since it doesnt
> actually verify the value of the cookies (hopefully me pending
> updates to rds-stress will provide better testing/examples in this
> space, because I actully have something multi-threaded, that must
> necesarily use a truly random value for the cookie, and must really
> make sure that the returned value from the kernel matches some
> cookie that is pending)

It might be nice to at least increment the variable on each
successful send. The test is single threaded anyway. And
then we can test that the returned values are in the defined
range.

> Also, coalescing some related comments for patch 6/7:
>
>> +       memset(&msg, 0, sizeof(msg));
>> +       msg.msg_name = &din;
>> +       msg.msg_namelen = sizeof(din);
>
> Not read, so no need to configure. In that case a simpler
> recv will do and is more concise than setting up recvmsg.
>
> Real RDS applications actually have to use recvmsg, since they
> can get cmsg info about other things like congestion notification
> so let's leave this as recvmsg - it does no harm, and provides
> test coverage for the recvmsg case as well.

Ah, okay. Yes, let's keep it as, then.