linux-kernel - Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

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: <200902161621.28142.oliver@neukum.org>
Date:	Mon, 16 Feb 2009 16:21:27 +0100
From:	Oliver Neukum <oliver@...kum.org>
To:	Mike Murphy <mamurph@...clemson.edu>
Cc:	linux-usb@...r.kernel.org, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

Am Monday 16 February 2009 14:22:05 schrieb Mike Murphy:
> >
> > 3. Possible memory leak in error case:
> >
> > +static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
> > +       struct xpad_data *data = NULL;
> > +       int check;
> > +
> > +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return NULL;
> > +
> > +       check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
> > +       if (check) {
> > +               kobject_put(&data->kobj);
> > +               return NULL;
> > +       }
> >
> 
> My understanding from Documentation/kobject.txt is that the
> kobject_put in the 2nd error check will set the kobj's reference
> counter to zero, eventually causing the kobject core to call my
> cleanup function for the ktype (xpad_release) and free the memory. Is
> this not correct? I find the sysfs docs to be fairly thin... and sysfs

I don't know. I also find that documentation thin.
Please add that the memory is freed elsewhere.

> seems to be substantially more complex than procfs or ioctls would be
> for the same purpose. However, everything I read suggested that sysfs
> is the "best" way to go in a modern kernel.
> 
> > 4. Why the cpup variety?
> >
> > +       coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));
> >
> 
> The cpup cast is in the original stable driver
> (drivers/input/joystick/xpad.c), and I didn't question it.
> 
> > 5. What happens if this work is already scheduled?
> >
> >        if (data[0] & 0x08) {
> > +               padnum = xpad->controller_data->controller_number;
> >                if (data[1] & 0x80) {
> > -                       xpad->pad_present = 1;
> > -                       usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> > -               } else
> > -                       xpad->pad_present = 0;
> > +                       printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
> > +                       xpad->controller_data->pad_present = 1;
> > +
> > +                       INIT_WORK(&xpad->work, &xpad_work_controller);
> > +                       schedule_work(&xpad->work);
> >

> 1. The user has to remove the battery pack from the controller,
> reinstall the battery pack, and re-activate the controller by pushing
> and holding the center button for at least 1 second.
> 
> 2. The kernel has to be busy enough not to have completed the work in
> the ~2 seconds a human could have done (1).

Also what happens if the work is scheduled  and you unplug?

> I need a bit of guidance from someone who has a better understanding
> of the work queues to have a good solution to this one. Is switching
> to PREPARE_WORK sufficient (with an INIT_WORK somewhere in
> xpad_probe)? Or is a more involved solution needed?
> 
> > 6. No GFP_ATOMIC. If you can take a mutex you can sleep.
> > +               usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
> >
> 
> Per the "Linux Device Drivers" book (O'Reilly, 3rd ed), the claim is
> made that submissions while holding a mutex should be GFP_ATOMIC. My

That is not correct. It is true while holding a spinlock, not a mutex.

> tests seemed to verify this claim... as sending LED commands
> GFP_KERNEL while holding the mutex resulted in BUGs (scheduling while
> atomic) in dmesg. Switching those GFP_KERNELs to GFP_ATOMICs
> eliminated that particular BUG.

Please post that BUG.

	Regards
		Oliver
--
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/