Fehler in saa7126_core?

[ChakaZulu]

Hi,

kann es sein, dass im saa-Treiber ein kleines Speicherloch ist?

Code: Alles auswählen

static int saa7126_detect_client(struct i2c_adapter *adapter, int address,
		unsigned short flags, int kind)

	struct i2c_client *client;
        [...]
	client = kmalloc(sizeof(struct i2c_client) +
			sizeof(struct saa7126), GFP_KERNEL);
       [...]
       	if (!encoder->devfs_handle) 
		return -EIO; // <-------- (1)

	if ((ret = i2c_attach_client(client))) {
		kfree(client); // <-------------- (2)
		return ret;
	}

Bei (1) wird überhaupt nichts freigegeben, bei (2) nur client. Letzteres ist dann auch in saa7126_detach() der Fall. Oder habe ich da was übersehen?

ciao,

ChakaZulu

[rasc]

sieht wohl so aus...

[Npq]

Ja, das ist mir beim Codereview für den 2.6er damals auch aufgefallen und im 2.6er-Zweig auch gefixt.

[ChakaZulu]

hi,

jaja, das kann ja jeder behaupten


ciao,

ChakaZulu

[gmo18t]

Hi,

nur der Vollständigkeit halber:
der 26er_branch vom saa kompiliert aber nur für "LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)" !
Im anderen Fall erfolgt eine "kfree(pmd)" -> siehe (B), obwohl gar kein "pmd" vorhanden -> siehe (A)!
Und an Stelle (B) sollte es deshalb eher "goto out_client;" heissen ...

Code: Alles auswählen

#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
	struct miscdevice *pmd = kcalloc(1,sizeof(struct miscdevice), GFP_KERNEL);  // <---- (A) pmd gibts nur für >= 2.5er !

	if (!pmd){
		ret = -ENOMEM;
		goto out_client;
	}

	pmd->name = (const char*)&encoder->name; /* entry must not change */
	pmd->fops = &saa7126_fops;
	pmd->minor = MISC_DYNAMIC_MINOR;
	if ((ret=misc_register(pmd))<0){
		printk("saa: unable to register device: Error %d\n",ret);
		goto out_pmd;
	}
	encoder->mdev = pmd;
	encoder->minor = pmd->minor;

#else
	encoder->devfs_handle = devfs_register (NULL, encoder->name, DEVFS_FL_DEFAULT, 0, 0,
			S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH,
			&saa7126_fops, encoder);

	if (!encoder->devfs_handle)
		ret = -EIO;
		goto out_pmd;                         // <---- (B) sollte eher "goto out_client;" heissen
#endif

	if ((ret = i2c_attach_client(client))<0){
		printk(KERN_ERR "saa: unable to attach client: Error %d\n",ret);
		goto out_reg;
	}

#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
	if ((ret = devfs_mk_cdev(MKDEV(MISC_MAJOR,pmd->minor), 
			S_IFCHR | S_IRUGO | S_IWUGO, "dbox/saa%d", encoder->id))) {
		goto out_reg;
	}
	list_add_tail(&encoder->lhead,&encoder_list);
#endif
	dprintk("[%s]: chip found @ 0x%x\n", __FILE__,  client->addr);

	saa7126_write_inittab(client, 1); /* init */
	/* set video mode */
	saa7126_set_mode(client, mode);

	return 0;
/* start of unwind block */
out_reg:
#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0)
		misc_deregister(pmd);
#else
		devfs_unregister(encoder->devfs_handle);
#endif
out_pmd:
		kfree(pmd);                           // <---- (C) pmd ist im "2.4er"-Fall nicht bekannt !
out_client:
		kfree(client);
		return ret;
}

- GMo -

[gmo18t]

Hi,

kann es sein, dass im saa-Treiber ein kleines Speicherloch ist?
...

hier eine Reparatur ohne "goto"

Code: Alles auswählen

static int saa7126_detect_client(struct i2c_adapter *adapter, int address,
		unsigned short flags, int kind)

	struct i2c_client *client;
        [...]
	client = kmalloc(sizeof(struct i2c_client) +
			sizeof(struct saa7126), GFP_KERNEL);
       [...]
	if (!encoder->devfs_handle)
		ret = -EIO;
	else
		ret = i2c_attach_client(client);

	if (ret != 0) {
		kfree(client); 
		return ret;
	}

- GMo -

[ChakaZulu]

hi,

das ist aber immer noch kaputt

Es wird ja dieser Speicher allokiert

Code: Alles auswählen

client = kmalloc(sizeof(struct i2c_client) +
         sizeof(struct saa7126), GFP_KERNEL); 

aber nachher nur von der Grösse i2c_client freigegeben.
Da ist dann noch der Speicher, der für client->data (ist vom Typ void*) allokiert wurde.

also sowas wie

Code: Alles auswählen

kfree((struct saa7126*)client->data);
kfree(client);

sollte doch eher geeignet sein.

ciao,

ChakaZulu

[Houdini]

er gibt den speicher frei, der zu dem pointer gehört, da hat die größe keine Auswirkungen.
zu einem malloc gibt es auch nur ein free

Houdini

[ChakaZulu]

hi,

hups. Ich war irgendwie der Meinung, dass beim free() die Grösse des Typs genommen wird und die Grösse, die beim malloc() angegeben wurde, nicht mehr bekannt ist. In diesem Fall hätte man aber auch schon beim malloc() die Typinformation benutzen können und die explizite Angabe der Grösse wäre unnötig gewesen. Also hätte mir schon deshalb klar sein sollen, dass meine Sicht falsch ist

ciao,

ChakaZulu

[Npq]

@gmo18t:
Hmm, ja, da hast du recht. Für den 2.4er ist das nicht getestet, wird dort auch überhaupt nicht kompilieren, weil dort unter anderem noch ein Header für i2c fehlt, der die Trickserei mit den client-Daten 2.6er-konform wrapped. Beim 2.6er gibt es nämlich dafür eine sichere[TM] Routine.

Das mit den ganzen #ifdefs ging damals auch nur solange gut wie der DVB-API-Teil kam.

Ich glaube mittlerweile, es wäre für die Lesbarkeit besser, wenn man eine reine 2.6er-Variante baut. Die Änderungen speziell bei den Makefiles bekommt man nämlich auch nicht wirklich schön in den 2.4er rein und das DVB-Subsystem ist auch ein anderes.

@Chakazulu: Die Trickserei mit dem kmalloc stört auch ein wenig mein "Ordnungsgefühl". Aber das wird sogar im offiziellen Kernel-Treiber so gehandhabt und hat auch einen guten Grund. Kmalloc allokiert immer mindestens eine Page, also 4 kByte. Daher ist es extrem ungünstig, viele kleine byteweise Allokierungen durchzuführen und man allokiert besser einen großen Block am Stück.