| 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
| ||
|
Message-ID: <fa78241556f71cafd6f0c553a1f8ff25.squirrel@webmail.it-technology.at>
Date: Sat, 18 Apr 2009 16:43:44 +0200 (CEST)
From: "Peter Holik" <peter@...ik.at>
To: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
Cc: "LKML" <linux-kernel@...r.kernel.org>,
"Netdev" <netdev@...r.kernel.org>, linux-usb@...r.kernel.org
Subject: Re: [Resend][PATCH] usb driver for intellon int51x1 based PLC like
devolo dlan duo
some fixes to my last mail:
>> This is a usb driver for the intellon int51x1 based PLC
>> (Powerline Communications) like devolo dlan duo.
>>
>> Signed-off-by: Peter Holik <peter@...ik.at>
>> ---
>> drivers/net/usb/Kconfig | 8 ++
>> drivers/net/usb/Makefile | 1 +
>> drivers/net/usb/int51x1.c | 268 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 277 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/net/usb/int51x1.c
>>
>> diff --git a/drivers/net/usb/int51x1.c b/drivers/net/usb/int51x1.c new file mode 100644
>> index 0000000..44a8a09
>> --- /dev/null
>> +++ b/drivers/net/usb/int51x1.c
>> @@ -0,0 +1,268 @@
>
>> +static int int51x1_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>> +{
>> + int len;
>> +
>> + if (unlikely(skb->len < INT51X1_HEADER_SIZE)) {
>
> pskb_may_pull(...)
ok, done:
static int int51x1_rx_fixup(struct usbnet *dev, struct sk_buff *skb) {
int len;
if (!(pskb_may_pull(skb, INT51X1_HEADER_SIZE))) {
deverr(dev, "unexpected tiny rx frame");
return 0;
}
len = le16_to_cpu(*(__le16 *)&skb->data[skb->len - 2]);
skb_trim(skb, len);
return 1;
}
>> + deverr(dev, "unexpected tiny rx frame");
>> + return 0;
>> + }
>> +
>> + len = le16_to_cpu(*(__le16 *)&skb->data[skb->len - 2]);
>> +
>> + skb_trim(skb, len);
>> +
>> + return 1;
>> +}
>> +
>> +static struct sk_buff *int51x1_tx_fixup(struct usbnet *dev,
>> + struct sk_buff *skb, gfp_t flags)
>> +{
>> + int pack_len = skb->len;
>> + int headroom = skb_headroom(skb);
>> + int tailroom = skb_tailroom(skb);
>> + int need_tail = 0;
>> + __le16 *len;
>> +
>> + /*
>> + * usbnet would send a ZLP if packetlength mod urbsize == 0 for us,
>> + * but we need to know ourself, because this would add to the length
>> + * we send down to the device...
>> + */
>> + if (!((pack_len + INT51X1_HEADER_SIZE) % dev->maxpacket))
>> + need_tail = 1;
>> +
>> + /* if packet and our header is smaler than 64 pad to 64 (+ ZLP) */
>> + if ((pack_len + INT51X1_HEADER_SIZE + need_tail) < dev->maxpacket + 1)
>> + need_tail = dev->maxpacket + 1 - pack_len - INT51X1_HEADER_SIZE;
>
> This is totally crazy code fragment, first need_tail is used like a boolean?
> But on the same some +1 scalar trick is being done?
> Is there some reason why DIV_ROUND_UP (linux/kernel.h)
> won't do what you want here and then you can trivally find diff = new - old ?
>
sorry i can't follow, can you convert this code to DIV_ROUND_UP
>> + if (!skb_cloned(skb) &&
>> + (headroom + tailroom >= need_tail + INT51X1_HEADER_SIZE)) {
>> + if (headroom < INT51X1_HEADER_SIZE || tailroom < need_tail) {
>> + skb->data = memmove(skb->head + INT51X1_HEADER_SIZE,
>> + skb->data, skb->len);
>> + skb_set_tail_pointer(skb, skb->len);
>> + }
>> + } else {
>> + struct sk_buff *skb2;
>> +
>> + skb2 = skb_copy_expand(skb,
>> + INT51X1_HEADER_SIZE,
>> + need_tail,
>> + flags);
>> + dev_kfree_skb_any(skb);
>> + if (!skb2)
>> + return NULL;
>> + skb = skb2;
>> + }
>> +
>> + pack_len += need_tail;
>> + pack_len &= 0x07ff;
>> +
>> + len = (__le16 *) __skb_push(skb, INT51X1_HEADER_SIZE);
>> + *len = cpu_to_le16(pack_len);
>> +
>> + if(need_tail)
>> + memset(__skb_put(skb, need_tail), 0, need_tail);
>> +
>> + return skb;
>> +}
>> +
>> +static void int51x1_async_cmd_callback(struct urb *urb)
>> +{
>> + struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
>> + int status = urb->status;
>> +
>> + if (status < 0)
>> + dev_warn(&urb->dev->dev, "async callback failed with %d\n", status);
>> +
>> + kfree(req);
>> + usb_free_urb(urb);
>> +}
>> +
>> +static void int51x1_set_multicast(struct net_device *netdev)
>> +{
>> + struct usb_ctrlrequest *req;
>> + int status;
>> + struct urb *urb;
>> + struct usbnet *dev = netdev_priv(netdev);
>> + u16 filter = PACKET_TYPE_DIRECTED |
>> + PACKET_TYPE_BROADCAST;
>
> Won't this fit on a single line?
>
ok, done
>> +
>> + if (netdev->flags & IFF_PROMISC) {
>> + /* do not expect to see traffic of other PLCs */
>> + filter |= PACKET_TYPE_PROMISCUOUS;
>> + devinfo(dev, "promiscuous mode enabled");
>> + } else if (netdev->mc_count ||
>> + (netdev->flags & IFF_ALLMULTI)) {
>> + filter |= PACKET_TYPE_ALL_MULTICAST;
>> + devdbg(dev, "receive all multicast enabled");
>> + } else {
>> + /* ~PROMISCUOUS, ~MULTICAST */
>> + devdbg(dev, "receive own packets only");
>> + }
>> +
>> + urb = usb_alloc_urb(0, GFP_ATOMIC);
>> + if (!urb) {
>> + devwarn(dev, "Error allocating URB");
>> + return;
>> + }
>> +
>> + req = kmalloc(sizeof *req, GFP_ATOMIC);
>
> sizeof(*req)
>
ok, done
>> + if (!req) {
>> + devwarn(dev, "Error allocating control msg");
>> + usb_free_urb(urb);
>> + return;
>
> I'd use gotos instead for error handling since you need similar call in the later
> error handling block too. Gotos make it somewhat harder to miss some necessary
> rollback actions in one of the error blocks (not that this case is buggy atm).
>
ok, done like you suggested - see at the end
>> + }
>> +
>> + req->bRequestType = USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE;
>> + req->bRequest = SET_ETHERNET_PACKET_FILTER;
>> + req->wValue = cpu_to_le16(filter);
>> + req->wIndex = 0;
>> + req->wLength = 0;
>> +
>> + usb_fill_control_urb(urb, dev->udev, usb_sndctrlpipe(dev->udev, 0),
>> + (void *)req, NULL, 0,
>> + int51x1_async_cmd_callback,
>> + (void *)req);
>> +
>> + status = usb_submit_urb(urb, GFP_ATOMIC);
>> + if (status < 0) {
>> + devwarn(dev, "Error submitting control msg, sts=%d", status);
>> + kfree(req);
>> + usb_free_urb(urb);
>
> Ditto.
static void int51x1_set_multicast(struct net_device *netdev)
{
struct usb_ctrlrequest *req;
int status;
struct urb *urb;
struct usbnet *dev = netdev_priv(netdev);
u16 filter = PACKET_TYPE_DIRECTED | PACKET_TYPE_BROADCAST;
if (netdev->flags & IFF_PROMISC) {
/* do not expect to see traffic of other PLCs */
filter |= PACKET_TYPE_PROMISCUOUS;
devinfo(dev, "promiscuous mode enabled");
} else if (netdev->mc_count ||
(netdev->flags & IFF_ALLMULTI)) {
filter |= PACKET_TYPE_ALL_MULTICAST;
devdbg(dev, "receive all multicast enabled");
} else {
/* ~PROMISCUOUS, ~MULTICAST */
devdbg(dev, "receive own packets only");
}
urb = usb_alloc_urb(0, GFP_ATOMIC);
if (!urb) {
devwarn(dev, "Error allocating URB");
return;
}
req = kmalloc(sizeof(*req), GFP_ATOMIC);
if (!req) {
devwarn(dev, "Error allocating control msg");
goto out;
}
req->bRequestType = USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE;
req->bRequest = SET_ETHERNET_PACKET_FILTER;
req->wValue = cpu_to_le16(filter);
req->wIndex = 0;
req->wLength = 0;
usb_fill_control_urb(urb, dev->udev, usb_sndctrlpipe(dev->udev, 0),
(void *)req, NULL, 0,
int51x1_async_cmd_callback,
(void *)req);
status = usb_submit_urb(urb, GFP_ATOMIC);
if (status < 0) {
devwarn(dev, "Error submitting control msg, sts=%d", status);
goto out1;
}
return;
out1:
kfree(req);
out:
usb_free_urb(urb);
}
cu Peter
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists