linux-kernel - Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation

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: <20130813.164118.1992860501009385037.davem@davemloft.net>
Date:	Tue, 13 Aug 2013 16:41:18 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	oneukum@...e.de
Cc:	hayeswang@...ltek.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation

From: Oliver Neukum <oneukum@...e.de>
Date: Tue, 13 Aug 2013 17:17:10 +0200

> On Tue, 2013-08-13 at 20:32 +0800, hayeswang wrote:
>>  Oliver Neukum [mailto:oneukum@...e.de] 
>> > Sent: Tuesday, August 13, 2013 4:49 PM
>> > To: Hayeswang
>> > Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org; 
>> > linux-usb@...r.kernel.org
>> > Subject: Re: [PATCH net-next 1/3] net/usb/r8152: support aggregation
>> > 
>> [...]
>> > > +               len_used = 0;
>> > > +               rx_desc = agg->head;
>> > > +               rx_data = agg->head;
>> > > +               smp_wmb();
>> > > +               pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
>> > > +               len_used += sizeof(struct rx_desc) + pkt_len;
>> > > +
>> > > +               while (urb->actual_length >= len_used) {
>> > > +                       if (pkt_len < ETH_ZLEN)
>> > > +                               break;
>> > > +
>> > > +                       pkt_len -= 4; /* CRC */
>> > > +                       rx_data += sizeof(struct rx_desc);
>> > > +
>> > > +                       skb = netdev_alloc_skb_ip_align(netdev,
>> > > pkt_len);
>> > > +                       if (!skb) {
>> > > +                               stats->rx_dropped++;
>> > > +                               break;
>> > > +                       }
>> > > +                       memcpy(skb->data, rx_data, pkt_len);
>> > > +                       skb_put(skb, pkt_len);
>> > > +                       skb->protocol = eth_type_trans(skb, netdev);
>> > > +                       netif_rx(skb);
>> > > +                       stats->rx_packets++;
>> > > +                       stats->rx_bytes += pkt_len;
>> > > +
>> > > +                       rx_data = rx_agg_align(rx_data + 
>> > pkt_len + 4);
>> > > +                       rx_desc = (struct rx_desc *)rx_data;
>> > > +                       smp_wmb();
>> > 
>> > Against what is the memory barrier?
>> 
>> Excuse me. I don't understand your question. Do you mean the function should not
>> be used here?
> 
> I don't understand what problem the function is supposed to fix. As long
> as I don't understand it I cannot say for sure whether it is correct.
> There seems no obvious reason for a memory barrier, but there may be a
> hidden reason I don't see.

Hayes, when Oliver asks you "Against what is the memory barrier?" he is asking
you which memory operations you are trying to order.

You do not explain this in your commit message, nor do you explain it with a
suitable comment.  This is not acceptable.

It is absolutely critical, that any time you add a memory barrier, you add a
comment above the new memory barrier explaining exactly what the barrier is
trying to achieve.

In fact, this is required by our coding standards.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/