RE: [PATCH 1/5] tty: n_gsm: Add raw-ip support

[Gorby, Russ]

>-----Original Message-----
>From: Alan Cox [mailto:alan@xxxxxxxxxxxxxxxxxxx]
>Sent: Monday, June 06, 2011 2:29 AM
>To: Gorby, Russ
>Cc: Greg Kroah-Hartman; linux-kernel@xxxxxxxxxxxxxxx; Ahmed, Suhail
>Subject: Re: [PATCH 1/5] tty: n_gsm: Add raw-ip support
>
>
>> @@ -1590,6 +1616,7 @@ static struct gsm_dlci *gsm_dlci_alloc(struct
>gsm_mux *gsm, int addr)
>>  	dlci->addr = addr;
>>  	dlci->adaption = gsm->adaption;
>>  	dlci->state = DLCI_CLOSED;
>> +	dlci->net = NULL;	/* network not initially created */
>
>kzalloc ised used anyway so this starts NULL
 
[Gorby, Russ] sorry, inherited code - I'll fix it.

>
>>
>> +int gsm_change_mtu(struct net_device *net, int new_mtu)
>> +{
>> +	if ((new_mtu < 8) || (new_mtu > MAX_MTU))
>> +		return -EINVAL;
>> +	net->mtu = new_mtu;
>> +	return 0;
>> +}
>
>Surely at that point you need to renegotiate the DLCI parameters for the
>DLCI in question. At the very least you need to sanity check versus the
>current settings ?

[Gorby, Russ] Thanks, I see we are misusing MAX_MTU, should be gsm->mtu instead

>
>
>> +static int gsm_create_network(struct gsm_dlci *dlci, struct
>gsm_netconfig *nc)
>> +{
>> +	char *netname;
>> +	int retval = 0;
>> +	struct net_device *net;
>> +	struct gsm_mux_net *mux_net;
>> +
>> +	if (!capable(CAP_NET_ADMIN))
>> +		return -EPERM;
>> +
>> +	/* Already in a non tty mode */
>> +	if (dlci->adaption > 2)
>> +		return -EBUSY;
>
>Only you don't have any locking so two ioctls at once will get very
>confused

 [Gorby, Russ] This is an ioctl against a gsmttyN  TTY to turn it into a network IF.
	The same ioctl on different nodes can proceed in parallel w/o problems (that I see). So the exclusion
	you're concerned with is from multiple clients of the TTY issuing this same ioctl at the same time?
	This would indicate a per-dlci exclusion needed then.

>
>
>> +	if (retval) {
>> +		pr_err("network register fail %d\n", retval);
>> +		free_netdev(net);
>> +		goto error_ret;
>
>And this leaves the DLCI messed up

[Gorby, Russ] how so? We don't modify the dlci until after this. We've just copied the mtu at this point.

>
>> +	kref_init(&mux_net->ref);
>
>What stops this getting referenced between the net register and here ?

[Gorby, Russ] OOPs - will fix

>
>On the destroy side you don't seem to put back the old adaption settings
>and restore the state (and also have locking v another ioctl missing)
>
>Doesn't look too hard to fix - save the old dlci states when you go
>network, put them back when you destroy the network state. For the
>ioctls
>you probably need to cover most of each one with the mutex.

 [Gorby, Russ] Thanks for the comments. I appreciate all the help.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/