Why this proc driver crashes?

Question

I am trying to write a proc driver that will print the driver history up till 10 last updates. In this driver, I haven't added the kernel data structure that I want to print. But, I am relying on the 'i' value to print the value up till 10. Here is my code.

#include <linux/init.h>
#include <linux/module.h> /** needed by all modules **/
#include <linux/kernel.h>  /** This is for KERN_ALERT **/
#include <linux/proc_fs.h>  /** This is for procfs **/
#include <linux/seq_file.h>
#include <linux/cdev.h>  /** character device **/
#include <linux/device.h>  /** for sys device registration in /dev/ and /sys/class **/

/** For class registration to work, you need GPL license **/
MODULE_LICENSE("GPL");


#define PROCFS_NAME  "basicProcfs1"

static struct cdev basicCdev;
static struct class *basicDriverClass;

struct proc_dir_entry *procFileEntry = NULL;

static int  basicMajorNumber = 0;

#define NUMBER_OF_MINOR_DEVICE (0)

#define NUM_MSG_HIST_ENTRIES (10)

static int gui32CmdMsgHistoryStartIndex=0;

/** This File operation table for proc file system **/

static int av_cmd_hist_show( struct seq_file *filp, void *v )
{
   int        i = *((int *)v);

    printk("the av_cmd_hist_show called\r\n");

    if ( i == 0)
    {
       seq_printf(filp, "Sequential print for debugging-- called for i-times i.e. 10 times \r\n");

    }

    seq_printf(filp, "Hello SJ proc! %d\r\n", i);

    return 0;

}

static void av_cmd_hist_stop( struct seq_file *filp, void *v )
{
  printk("av_cmd_hist_stop called..\r\n");
} /* av_cmd_hist_stop */


static void *av_cmd_hist_next( struct seq_file *filp, void *v, loff_t *pos )
{
   (*pos)++;
   printk("av_cmd_hist_next called..\r\n");
   return( ( *pos < NUM_MSG_HIST_ENTRIES ) ? pos : NULL );

 } /* av_cmd_hist_next */


static void *av_cmd_hist_start( struct seq_file *filp, loff_t *pos )
{
    if( *pos == 0 )
    {
        printk("av_cmd_hist_start.. Initial..\r\n");
        gui32CmdMsgHistoryStartIndex = 5;
    }

    printk("av_cmd_hist_start.. the *pos=0..\r\n");

    return( ( *pos < NUM_MSG_HIST_ENTRIES ) ? pos : NULL );

} /* av_cmd_hist_start */


static struct seq_operations av_seq_cmd_hist_fops =
{
 .start = av_cmd_hist_start,
 .next  = av_cmd_hist_next,
 .stop  = av_cmd_hist_stop,
 .show  = av_cmd_hist_show
};


static int basicProcShow(struct seq_file *m, void *v) {
  seq_printf(m, "Hello SJ proc!\n");
  return 0;
}

static int basicProcOpen(struct inode *inode, struct file *file)
{
   int i;

   i = seq_open( file, &av_seq_cmd_hist_fops ); 

   return  i;
  //return single_open(file, basicProcShow, NULL);  

}

/** Put data into the proc fs file **/

static const struct file_operations basic_proc_fops =
{

  .owner = THIS_MODULE,
  .open = basicProcOpen,
  .read = seq_read,
  .llseek = seq_lseek,
  .release = single_release,    

};


static struct file_operations fops = {
  .read = NULL,
  .write = NULL,
  .open = NULL,
  .release = NULL
};


static void setup_cdev(struct cdev *dev, int minor, struct file_operations *fops)
{
         int err = -1;
         /** MKDEV call creates a device number i.e. combination of major and minor number **/
         int devno = MKDEV(basicMajorNumber, minor);
         /** Initiliaze character dev with fops **/
         cdev_init(dev, fops);
         /**owner and operations initialized **/
         dev->owner = THIS_MODULE;
         dev->ops = fops;
         /** add the character device to the system**/
         /** Here 1 means only 1 minor number, you can give 2 for 2 minor device, the last param is the count of minor number enrolled **/
         err = cdev_add (dev, devno, 1);

         if (err)
         {
                 printk (KERN_NOTICE "Couldn't add cdev");
         }
 }

static int chrDriverInit(void)
{

    int result;
    dev_t dev; 

    printk("Welcome!! Device Init now..");

    /** int alloc_chrdev_region(dev_t *dev, unsigned int firstminor,unsigned int count, char *name);  **/
    /** dev -> The dev_t variable type,which will get the major number that the kernel allocates.  **/
    /**The same name will appear in /proc/devices.  **/

    /** it is registering the character device **/   
    /** a major number will be dynamically allocated here **/
    /**  alloc_chrdev_region(&dev_num, FIRST_MINOR, COUNT, DEVICE_NAME); **/
    result = alloc_chrdev_region(&dev, 0, NUMBER_OF_MINOR_DEVICE, "pSeudoDrv1");

    if( result < 0 )
    {
      printk("Error in allocating device");
      return -1;    
    }

    /** From these two if's we are avoiding the manual mknod command to create the /dev/<driver> **/
    /**  creating class, and then device created removes the dependency of calling mknod  **/
    /** A good method - the mknod way is depreciated **/
    /** mknod way is -  mknod /dev/<driver_name> c <majorNumber> <minorNumber>


    /** add the driver to /sys/class/chardrv **/
    if ((basicDriverClass = class_create(THIS_MODULE, "chardrv")) == NULL)    //$ls /sys/class
    {
        unregister_chrdev_region(dev, 1);
        return -1;
    }

     /** add the driver to /dev/pSeudoDrv -- here **/
    if (device_create(basicDriverClass, NULL, dev, NULL, "pSeudoDrv") == NULL) //$ls /dev/
    {
        class_destroy(basicDriverClass);
        unregister_chrdev_region(dev, 1);
        return -1;
    }

     /** let's see what major number was assigned by the Kernel **/
     basicMajorNumber = MAJOR(dev);
     printk("Kernel assigned major number is %d ..\r\n",basicMajorNumber );

    /** Now setup the cdev **/
    setup_cdev(&basicCdev,NUMBER_OF_MINOR_DEVICE, &fops);

    /** Setup Proc Entry here **/
    /** 0644 means - 
     * 0 - owning (user) : read and write - 110 
     * Group - only read - 100
     * Other - only read - 100 **/


    procFileEntry = proc_create(PROCFS_NAME, 0, NULL, &basic_proc_fops);

    if ( procFileEntry == NULL)
    {
       remove_proc_entry(PROCFS_NAME, NULL);    

    }


    return 0;   

}


static void chrDriverExit(void)
{
    /** A reverse - destroy mechansim -- the way it was created **/
    printk("Releasing Simple Devs -- %s\r\n",  __FUNCTION__);
    /** delete the character driver added **/
    cdev_del(&basicCdev);
    /** destroy the device created **/
    device_destroy(basicDriverClass, MKDEV(basicMajorNumber, 0));
    /** destroy the class created **/
    class_destroy(basicDriverClass);
    /** unregister the chr dev **/
    unregister_chrdev(basicMajorNumber, NUMBER_OF_MINOR_DEVICE);
    remove_proc_entry(PROCFS_NAME, NULL);

}


module_init(chrDriverInit);
module_exit(chrDriverExit);

The dmesg logs are the following.

# dmesg
[14102.921743] Releasing Simple Devs -- chrDriverExit
[14163.285107] Welcome!! Device Init now..Kernel assigned major number is 244 ..
[14174.979098] av_cmd_hist_start.. Initial..
[14174.979103] av_cmd_hist_start.. the *pos=0..
[14174.979104] the av_cmd_hist_show called
[14174.979107] av_cmd_hist_next called..
[14174.979108] the av_cmd_hist_show called
[14174.979109] av_cmd_hist_next called..
[14174.979110] the av_cmd_hist_show called
[14174.979112] av_cmd_hist_next called..
[14174.979113] the av_cmd_hist_show called
[14174.979114] av_cmd_hist_next called..
[14174.979115] the av_cmd_hist_show called
[14174.979117] av_cmd_hist_next called..
[14174.979118] the av_cmd_hist_show called
[14174.979119] av_cmd_hist_next called..
[14174.979120] the av_cmd_hist_show called
[14174.979121] av_cmd_hist_next called..
[14174.979122] the av_cmd_hist_show called
[14174.979124] av_cmd_hist_next called..
[14174.979125] the av_cmd_hist_show called
[14174.979126] av_cmd_hist_next called..
[14174.979127] the av_cmd_hist_show called
[14174.979128] av_cmd_hist_next called..
[14174.979130] av_cmd_hist_stop called..
[14174.979231] av_cmd_hist_start.. the *pos=0..
[14174.979233] av_cmd_hist_stop called..
[14174.979250] ------------[ cut here ]------------
[14174.979252] kernel BUG at mm/slub.c:3483!
[14174.979254] invalid opcode: 0000 [#2] SMP 
[14174.979258] Modules linked in: procfs_driver1(O) procfs_driver(O-) tcp_lp nfsv3 nfsv4 nfs fscache dns_resolver fuse vboxpci(O) vboxnetadp(O) vboxnetflt(O) 8021q garp stp llc binfmt_misc vboxdrv(O) tpm_bios snd_hda_codec_hdmi snd_hda_codec_realtek fglrx(PO) snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm iTCO_wdt iTCO_vendor_support r8169 snd_timer mii e1000e snd i2c_i801 lpc_ich i2c_core soundcore snd_page_alloc coretemp kvm_intel kvm serio_raw video dcdbas microcode uinput nfsd lockd nfs_acl auth_rpcgss sunrpc crc32c_intel [last unloaded: procfs_driver1]
[14174.979297] Pid: 19055, comm: cat Tainted: P    B D  C O 3.6.11-4.fc16.i686 #1 Dell Inc. OptiPlex 9010/00F82W
[14174.979300] EIP: 0060:[<c0530891>] EFLAGS: 00210246 CPU: 5

The proc output is the following -

# cat /proc/basicProcfs1 
Sequential print for debugging-- called for i-times i.e. 10 times 
Hello SJ proc! 0
Hello SJ proc! 1
Hello SJ proc! 2
Hello SJ proc! 3
Hello SJ proc! 4
Hello SJ proc! 5
Hello SJ proc! 6
Hello SJ proc! 7
Hello SJ proc! 8
Hello SJ proc! 9
Segmentation fault

Answer 1

Sorry, I got the fix.

Erroneous code

static const struct file_operations basic_proc_fops =
{

  .owner = THIS_MODULE,
  .open = basicProcOpen,
  .read = seq_read,
  .llseek = seq_lseek,
  .release = single_release,    

};

Right code-

 static const struct file_operations basic_proc_fops =
    {

      .owner = THIS_MODULE,
      .open = basicProcOpen,
      .read = seq_read,
      .llseek = seq_lseek,
      .release = seq_release,    

    };

I would appreciate, if someone can explain why it makes the difference.

Answer 2

It is crashing becasue of this return( ( *pos < NUM_MSG_HIST_ENTRIES ) ? pos : NULL );

Answer 3

(I do not have enough reputation to comment, so adding it as an answer) Regarding your question on why seq_release and not single_release, 'release' functions should compliment 'open' functions. Your open function here 'basicProcOpen' calls 'seq_open' so you should be calling seq_release.

If you look at single_release, in addition to seq_release, there is a kfree(op) which causes your crash.

623 int single_release(struct inode *inode, struct file *file)
624 {
625         const struct seq_operations *op = ((struct seq_file *)file->private_data)->op;
626         int res = seq_release(inode, file);
627         kfree(op);
628         return res;
629 }
630 EXPORT_SYMBOL(single_release);